CODEWITHSUNDEEP
carraysstruct
syntagma
syntagma

Reputation: 24294

Memory allocation of fixed size array inside a struct

I have the following tree node struct that holds pointers to other tree nodes: 

struct node {
    // ...
    struct node* children[20];
}

The idea is that I want to check whether there is node* inside the children and based and that go deeper into the tree. So when I allocate the node I want to have children with 20 NULL values. Currently I am not doin

  1. How should I allocate this array in order to not get errors like Conditional jump or move depends on uninitialised value(s) (Valgrind)?
  2. Would it be better to use struct node** children and allocate fixed size each time I allocate a new node?

EDIT: Example of one place where Valgrind complains:

for(int i=0;i<20;i++)
  if(node->children[i] != NULL)
    do_something_with_the_node(node->children[i]);

Upvotes: 0

Views: 1248

Answers (3)

unwind
unwind

Reputation: 399703

When you allocate a new instance of struct node, you must set the contained pointers to NULL to mark them as "not pointing anywhere". This will make the Valgrind warning go away, since the pointers will no longer be uninitialized.

Something like this:

struct node * node_new(void)
{
  struct node *n = malloc(sizeof *n);
  if(n != NULL)
  {
    for(size_t i = 0; i < sizeof n->children / sizeof *n->children; ++i)
      n->children[i] = NULL;
  }
  return n;
}

You cannot portably use either memset() on n->children nor calloc(), since those will give you "all bits zero" which is not the same as "pointer NULL".

Upvotes: 1

rubikonx9
rubikonx9

Reputation: 1413

The problem is that you are using an unintialized value in an if condition.

When you instantiate a struct node, its member struct node* children[20]; is an array of 20 struct node *, all of which are uninitialized.

It would be no different from this:

char *x;

if (x == NULL) {
    /* Stuff */
}

At this point, x may have literally any value. In your example, any element of an array may have any value.

To fix this, you need to initialize the elements of an array before using them, for example like this:

for (int i = 0; i < 20; ++i) {
    node->children[i] = NULL;
}

Or shorter:

memset(node->children, 0, 20);

If you changed the member to, as you've suggested, node **children, the situation wouldn't be much different - you'll still need to initialize all the members, including array's elements. You could make it shorter by using calloc, which will initialize all bytes to 0; then again, you'll need some code for correct deallocation (and remember to do it), so I think the tradeoff's not worth it.

Upvotes: 0

mfro
mfro

Reputation: 3335

Your struct definition is valid (although it's hard to tell without more context if it fits your requirements).

Valgrind doesn't complain about your struct definition, it probably complains about how you instantiate variables of that type. Ensure that all of the array members get initialized and the complaints will most likely go away.

Upvotes: 0

Related Questions