Incompatible pointer type error using structs

Question

I am new to programming. I am trying to learn C and pointers, but it is giving me much trouble. I got the following error trying to implement a singly linked list. I searched online, and I couldn't find someone who had an error just like mine, or perhaps I just didn't couldn't make sense of it with my problem.

The following is the error I received:

warning: incompatible pointer types initializing 'NODE *' (aka 'struct node *') with an expression of type 'struct NODE ' [-Wincompatible-pointer-types] NODE temp = (*l)->head;

    NODE* temp = (*l)->head;

In main, I passed the address of the variable of type LIST. So, I thought I had to dereference 'l', to get the address of where the LIST type is located, then I had to dereference with an arrow to get the address of where the NODE is located. Where am I confused? I do appreciate the help.

Below you will see the code I have written:

typedef struct node {
    int value;
    struct node* next;
}NODE;

typedef struct list{
    struct NODE* head;
}LIST;

void insert(LIST** l, int x){

    if((*l)->head == NULL){

      NODE* new_Node = (NODE*) malloc(sizeof(NODE));
      new_Node->next = NULL;
      new_Node->value = x;
    }

    NODE* temp = (*l)->head;

    while(temp->next != NULL){
      temp=temp->next;
    }

    NODE* new_Node = (NODE*) malloc (sizeof(NODE));
    temp->next = new_Node;
    new_Node->next = NULL;
    new_Node->value = x;
}

int main(){

    LIST *l = (LIST*) malloc(sizeof(LIST));

    insert(&l, 5);

    return 0;
}

Solution 1

I guess your problem is here :

typedef struct list
{
     struct NODE* head;
}LIST;

just remove struct keyword before NODE

typedef struct list
{
     NODE* head;
}LIST;

or

typedef struct list
{
     struct node* head;
}LIST; 

Also you need to initialize the head with NULL to make this condition to wwork

  if((*l)->head == NULL) .....

so when you create your list add l->head = NULL;

      LIST *l = malloc(sizeof(LIST));
      l->head = NULL;

And the last one (i hope) when you create your first node, you forget to assign head to it, and return in order not to add the first element twice

      if((*l)->head == NULL)
      {

         NODE* new_Node = malloc(sizeof(NODE));
         new_Node->next = NULL;
         new_Node->value = x;
         (*l)->head = new_Node;
         return;
      }

And BTW, don't cast malloc results in C

Solution 2

This:

typedef struct list{
    struct NODE* head;
}LIST;

Should be this:

typedef struct list{
    NODE* head;
}LIST;

Tested and compiles fine with that change.

Solution 3

Your use of *l is correct. The problem is with the line:

NODE* temp = (*l)->head;

The left-hand side is NODE *, which is the same as struct node *, however the right-hand side is struct NODE *.

C is case-sensitive, struct node and struct NODE are different types. Also, the namespace of struct tags is separate to that of other types, so NODE and struct NODE are also different types.

I think you meant, in LIST's definition, that struct NODE* head; should be NODE* head;. There is no warning generated on that line, because in C it's legal to implicitly declare a struct type just by mentioning it (i.e. this line declares the new type struct NODE also).

Comments

[Apprentice0]

That was the error. It compiled without errors; now, I have a "segmentation fault: 11" that I did not expect.

[M.M]

Your code has logic bugs, try working on that. The first one is that you never initialize the values in *l. Pointers don't automatically start off as NULL or anything.

[Apprentice0]

Thanks for your help. That was the error. Unfortunately, I traded one error for another. I have a segfault now that I have to find.

[Dabo]

@Apprentice0 segfault on which line ? Can you debug ?

[Apprentice0]

I am learning how to use GDB. I will attempt to run the executable with it.

[Apprentice0]

I got this using GDB, not sure if this will help: * thread #1: tid = 0x31a93d, 0x0000000100000ecd a.outinsert + 93, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8) frame #0: 0x0000000100000ecd a.outinsert + 93 a.out`insert + 93: -> 0x100000ecd: cmpq $0x0, 0x8(%rax) 0x100000ed5: je 0x100000eec ; insert + 124 0x100000edb: movq -0x20(%rbp), %rax 0x100000edf: movq 0x8(%rax), %rax

[M.M]

You probably also want to set (*l)->head to the new node you just allocated , and return after doing so (in the if((*l)->head == NULL) block)

[Apprentice0]

Wow, I completely goofed up there. I forgot to assign the new node in that block. Thanks @Matt McNabb I have no segfaults now.

[M.M]

consider up-voting helpful answers and accepting one

[Apprentice0]

Thanks, those were the problems. I assigned it in the other block, but somehow I missed it in the if((*l)->head == NULL) block. I didn't include the return statement, even though it compiled. So, I am guessing that two new node would be created with identical values before it left that function.

[Apprentice0]

I am confused on why you are using *new_Node after size0f in the following line: NODE *new_Node = malloc(sizeof(*new_Node)); I thought that creating memory for the struct new_Node would go there instead of a pointer type to new_Node? I would appreciate you helping me understand that part. Yeah, I was redundant with the code. Thanks for showing me that inefficiency of mine.

[ajay]

That's because sizeof will infer the type of (*new_Node). Also, it's more maintainable and avoids repeating type. If you were to change the definition of the type NODE, the malloc statement would still work without the need to change anything.

[Apprentice0]

Thanks @ajay. Definitely will use that approach from this moment forward.

[Apprentice0]

I just signed up, and apparently I can't up vote at the moment. But, I will keep what you said in mind. I received much help with this problem, and I appreciate everyone's help.