1

I am writing a Bluetooth Mesh application using the nRF51 chip from Nordic Semiconductor and while looking at some examples I stumbled onto this pieces of code:

nrf_mesh_address_t address = {NRF_MESH_ADDRESS_TYPE_INVALID, 0, NULL};
                   address.type = NRF_MESH_ADDRESS_TYPE_GROUP;
                   address.value = GROUP_ADDRESS;

As i read this snippet address is first defined with some variables which is immediately changed again. Isn't this double defining unnecessary?

The definition of nrf_mesh_addrees_t look like this:

typedef struct
{
    nrf_mesh_address_type_t type;
    uint16_t value;
    const uint8_t* p_virtual_uuid;
} nrf_mesh_address_t;

My first thought was that p_virtual_uuid was declared const so it could not be changed after the definition, but i did not get a compile error when changing the code to this:

nrf_mesh_address_t address;
                   address.type = NRF_MESH_ADDRESS_TYPE_GROUP;
                   address.value = GROUP_ADDRESS;
                   address.p_virtual_uuid = NULL;

Are there any advantages to defining the variable twice with different variables?

SørenHN
  • 566
  • 5
  • 20
  • 3
    *p_virtual_uuid was declared const*. No, it is declared as a pointer to `const` – Gaurav Sehgal Oct 23 '17 at 10:22
  • Maybe some coding guildelines demand all variables to be initialized. Strictly following such rules sometimes leads to strange code.... – Gerhardh Oct 23 '17 at 10:26
  • Maybe `NRF_MESH_ADDRESS_TYPE_GROUP` and `GROUP_ADDRESS` are macros whose expansions are not constant expressions? C89 required constant expressions in a struct initializer list, but C99/C11 allow any expression. – aschepler Oct 23 '17 at 10:33
  • For debugging purposes, this code can quickly have the values toggled by adding `//` at the start of the later lines – M.M Oct 23 '17 at 10:56

3 Answers3

6

Yes, that seems very redundant.

If you're using C99, I'd suggest:

nrf_mesh_address_t address = { .type = NRF_MESH_ADDRESS_TYPE_GROUP,
                               .value = GROUP_ADDRESS,
                               .p_virtual_uuid = NULL };

The const refers to the data being pointed at, not the pointer itself. The latter would be uint8_t * const p_virtual_uuid.

unwind
  • 391,730
  • 64
  • 469
  • 606
  • Since OP has beginner-type questions, it might be worth including a link explaining [why this works and inits virtual uuid to 0](https://stackoverflow.com/a/10828333/3212865). – spectras Oct 23 '17 at 10:25
  • ... well we don't even know if this was static storage duration or not... – Antti Haapala -- Слава Україні Oct 23 '17 at 10:25
  • Are there any advantages by doing the initialization like this instead of the way i have done it? – SørenHN Oct 23 '17 at 10:45
  • @SørenHN, yes. You should always initialize, because this also sets members to `0` that you don't know, forgot about, or are added later during development. Otherwise these can end up having undetermined values, and in the worst, crash your program. – Jens Gustedt Oct 23 '17 at 10:59
2

The data member

const uint8_t* p_virtual_uuid;

is not a constant data member. It is a non-constant pointer that points to constant object.

A declaration of a constant pointer that points to constant object will look like

const uint8_t * const p_virtual_uuid;

In this case you indead have ro initialize the data member when an object of the structure type is defined.

The expression statements that follow the declaration in this code snippet

nrf_mesh_address_t address = {NRF_MESH_ADDRESS_TYPE_GROUP, 0, NULL};
                   address.type = NRF_MESH_ADDRESS_TYPE_GROUP;
                   address.value = GROUP_ADDRESS;

are redundant.

You could just write

nrf_mesh_address_t address = {NRF_MESH_ADDRESS_TYPE_INVALID, GROUP_ADDRESS, NULL};

Or you could use designators in the initializer list. For exasmple

nrf_mesh_address_t address = 
{ 
    .type = NRF_MESH_ADDRESS_TYPE_INVALID, 
    .value = GROUP_ADDRESS, 
    .p_virtual_uuid = NULL
};

Opposite to C++ there is one problem with braced list initializers in C. Evaluations of initializating expressions are unsequenced. So if the value of one initioalizer depends on the value of another initializer then indeed you will need to use assignments instead of initializing expressions in a braced-enclosed list.

From the C Standard (6.7.8 Initialization)

23 The order in which any side effects occur among the initialization list expressions is unspecified.

Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
1

You are correct in that they are redundant and it's equivalent to your last code snippet.

However I do see a potential value for the first use. For instance if you want to enforce a policy of always initializing variables. You could have the default initialization

nrf_mesh_address_t address = {NRF_MESH_ADDRESS_TYPE_INVALID, 0, NULL};

available and ready to be copy-pasted.

This can avoid situations like this:

nrf_mesh_address_t address;
                   address.type = NRF_MESH_ADDRESS_TYPE_GROUP;
                   address.value = GROUP_ADDRESS;

I wanted to set type and value and leave p_virtual_uuid to the default value. However I forgot to initialize it and so now I have bug in my code.

The value of this practice is however debatable. I just wanted to show a possible valid motivation.

bolov
  • 72,283
  • 15
  • 145
  • 224
  • Thank you for the answer, it is nice to know that there might be some kind sane thought behind it ;) – SørenHN Oct 23 '17 at 10:30