0

I'm sorry for too specific question, but pointer stunts are very difficult to me, with my Py background.

Edited

I want to declare an array of structures:

struct key {         
  int* data;         
  struct node* left; 
  struct node* right;
};                   

//There is a problem too. Is it possible to return an array of structs from a function.                 
struct key *NewNode(int value) { 
  struct key** new_node;                          
  struct key* new_key;                            

  new_node = (struct key*)malloc(sizeof(new_key)); // warning is here
  new_key  = malloc(sizeof *new_key);             

  new_key->data = value;                          
  new_key->left = NULL;                           
  new_key->right = NULL;                          

  new_node[0] = new_key;                          

  return new_node;                      
}                 

// Somewhere in further code: 
realloc(new_node, sizeof *key);
new_node[1] = new_key; // Next key passed to the node.

How to declare array of structs without this warning?

assignment from incompatible pointer type

And please, describe to me, why is it happened? Because I still believe that the type of array members is struct key.

I159
  • 29,741
  • 31
  • 97
  • 132
  • `new_node` is a pointer to pointer, hence the warning. Can you not just return `new_key`? – AntonH Apr 01 '14 at 20:52
  • You are assigning a `struct key *` to a `struct key **`. If you want to do this you need to dereference your double pointer: `*new_node = (struct key*)malloc(sizeof(new_key));` You also need to initialize your double pointer. – Hunter McMillen Apr 01 '14 at 20:53
  • 1
    And [don't cast the result of malloc](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) either! – Paul R Apr 01 '14 at 20:54
  • 1
    @HunterMcMillen Does that work, given that you don't know value that `*new_node` points to? – AntonH Apr 01 '14 at 20:54
  • @AntonH I didn't notice it wasn't initialized. So no, in this case it doesn't because the double pointer isn't initialized, but presumably it would work if it was. – Hunter McMillen Apr 01 '14 at 20:56
  • @HunterMcMillen Thank goodness, I as worried you were using some esoteric technique I wasn't aware of :D – AntonH Apr 01 '14 at 20:57
  • For `malloc` of `new_node`, you would use `sizeof(new_node)`. – AntonH Apr 01 '14 at 20:58
  • I've exhausted efforts trying to find `struct node`, used for the `left` and `right` members of `struct key`. And @AntonH, for `malloc` of **any** pointer-to type, you can always use `sizeof(*pvar)` where `pvar` is the typed pointer variable. Your "you would use" is wrong. It should be `new_node = malloc(sizeof(*new_node));` – WhozCraig Apr 01 '14 at 21:07
  • @WhozCraig My point was using variable `new_key` to determine the size of `new_node`. But you are correct about `*new_node`, I forgot it was a pointer to pointer. – AntonH Apr 01 '14 at 21:10
  • @AntonH Ah, I see (I think), but pointer-to-pointer has nothing to do with my earlier comment. It could just as easily be a single indirection pointer as well, `Type *p = malloc(sizeof(*p));` for an item allocation, `Type *p = malloc(sizeof(*p)*N);` for a sequence allocation, uses the identical syntax, does the correct thing, and is thus the reason why that syntax for allocation is highly recommended. Whether a pointer, a pointer-to-pointer, etc, makes no difference. – WhozCraig Apr 01 '14 at 21:15

2 Answers2

2

There are several problems here.

First of all, I'm assuming that the purpose of this code is to allocate and initialize a single object of type struct node; as such, new_node is superfluous; it serves no purpose. Just get rid of it completely and return new_key directly.

Secondly, 

new_key  = malloc(sizeof(new_key));

won't allocate enough memory to store an object of type struct node; it only allocates enough space to store a pointer to struct node, because the type of the expression new_key is struct node *. You'll want to change that line to

new_key = malloc( sizeof *new_key );

Note that sizeof is an operator, not a function; parens are only necessary if the operand is a type name like int or float or struct whatchamacallit.

Finally, the line

new_key->data = value;

is a red flag; depending on how you call NewNode, this pointer may wind up being the same for all of your nodes. If your caller looks something like:

while ( not_done_yet )
{
  x = get_input();
  node = NewNode( &x );
  root = insert( root, node );
}

all of the nodes in your tree will wind up pointing to the same thing (x). Worse yet, if x goes out of scope, then suddenly all those pointers become invalid.

If your node is meant to store a single integer value, then it's better to make both the parameter and the data member regular ints, rather than pointers to int.

Summarizing:

struct node {         
  int data;         
  struct node* left; 
  struct node* right;
};                   

struct node *NewNode(int value) 
{
  struct node* new_key = malloc(sizeof *new_key);
  if ( new_key )
  {
    new_key->data = value;
    new_key->left = NULL;
    new_key->right = NULL;
  }
  return new_key;  
}

EDIT

If you want to create an array of nodes, do so in a function that calls NewNode, not in the NewNode function itself:

#define INITIAL_SIZE 1

struct node *CreateNodeArray( size_t *finalArraySize )
{
  *finalArraySize = 0;
  size_t i = 0;

  /**
   * For the purpose of this example, we'll start with an array of size
   * 1 and double it each time we hit the limit, but in practice you'd
   * want to start with a minimum size large enough to handle most
   * cases.
   */
  struct node *new_node = malloc( sizeof *new_node * INITIAL_SIZE );
  if ( new_node )
  {
    *finalArraySize = INITIAL_SIZE;

    while ( there_is_more_data() )
    {
      /**
       * First, check to see if we've hit the end of our array
       */
      if ( i == *finalArraySize )
      {
        /**
         * We've hit the end of the array, so we need to extend
         * it; in this case, we'll double its size
         */
        struct node *tmp = realloc( new_node, 
                                    sizeof *new_node * (*finalArraySize * 2) );              
        if ( tmp )
        {
          *finalArraySize *= 2;
          new_node = tmp;
        }
      }

      /**
       * Add a new node to the array
       */
      int data = getInput();
      new_node[i++] = NewNode( data ); 
    }
  }
  return new_node;
}

This code makes some assumptions about how you're building the individual nodes, but it should illustrate the point I'm trying to make, which is that you want cleanly separate building the array of nodes from building a single node.

John Bode
  • 119,563
  • 19
  • 122
  • 198
  • +1 (but I still want to know where `struct node*` comes into this). – WhozCraig Apr 01 '14 at 22:43
  • @WhozCraig: Oh for crap's sake, that snuck right by me. I plead lack of sleep. – John Bode Apr 02 '14 at 00:30
  • @JohnBode, your answer makes my understanding much more clearer. But before I'll vote your answer up... I need to a `new_node` be an array of structs `key`. New node got only one member, but later amount of keys could be increased. I returned pointer to the `0` elem, because I don't know how to return an array of structs from a function. I edited my question. – I159 Apr 02 '14 at 05:33
1

The warning comes from your bogus cast. Remove it, you should not cast the value returned by malloc anyway.

Also, you malloc the wrong number of bytes to new_key on the line after.

You could have avoided both of these errors by using this idiom:

new_node = malloc( sizeof *new_node );
new_key = malloc( sizeof *new_key );

Your function also has a memory leak. You return new_node[0], leaking the memory new_node. I'm not exactly sure what you're trying to do (I'd expect that a function called "new node" would return a new struct node); but if you remove new_node entirely from the function then it still works just the same and doesn't leak memory.

M.M
  • 138,810
  • 21
  • 208
  • 365