Simple delete node singly linked list in C

[reza.adinata]

Hi all,

I am new in C programming, currently learning about linked list. I know that there is a lot of examples about linked list, but I decided to write my own code, to make me more understand. I managed to build a linked list, insert a new node after one cell, but I have a segmentation fault after deleting one node.

Can anyone help me with this, why I got segmentation fault ?

The code snippet (just for the delete block)

void cell_delete(int a)
{
cell *p, *q;
p=header.next;
q=&header;
printf("you are before while block\n");

	while((p!=NULL))
	{
	q=p;
	p=p->next;
		if(p->value = a)
		{	
			printf("correct value\n");
			p=q->next;
			q->next = p->next;	
			print_list();
			free(p);
			free(q);
		}
		else
		{
			printf("value of p is wrong");
		}
	}
		
printf("you are OUT OF while block\n");
}

[Narue]

>if(p->value = a)
Two things:Comparison is done with the == operator. The = operator is for assignment. You're doing the equivalent of this:

p->value = a;
if (p->value != 0)

You've already advanced p at this point, so how do you know it's not NULL?

[reza.adinata]

>if(p->value = a)[*]You've already advanced p at this point, so how do you know it's not NULL?[/list]

Thank you for your answer and thank you for the == operator.
I am kinda confused with what you ask, AFAIK, the p will traverse from one node till the last node (which is NULL), that is why I write do the code below while (p!=NULL) .. Am I understanding you well?

[Narue]

Your loop looks like this:

while (p != NULL)
{
    /* p is definitely not NULL, so this is safe */
    p = p->next;

    /* If p->next was NULL, p is now NULL, so this is *not* safe */
    if (p->value == a)
    {
        ...
    }
    else
    {
        ...
    }
}

See the problem?

[Schol-R-LEA]

The problem is that you are changing the meaning of p before you test p->value, which means that you don't know yet if p (and hence p->value) is valid. You want to test q->next, not p->next:

while((p!=NULL))
	{
	q=p;
        p = p->next;
		if(q->value = a)
		{	
			printf("correct value\n");
			p=q->next;
			q->next = p->next;	
			print_list();
			free(p);
			free(q);
		}
		else
		{
			printf("value of p is wrong");
		}
	}

PS: You're indent style is unusual; is that by intention?

[reza.adinata]

The problem is that you are changing the meaning of p before you test p->value, which means that you don't know yet if p (and hence p->value) is valid. You want to test q->next, not p->next:

PS: You're indent style is unusual; is that by intention?

Yes thank you everyone about pointing out
q=p;
p = p->next; //if p = NULL, it cause segmentation fault. I did not aware of it ..

but I am sorry again, I don't understand what do you mean with "you want to test q->next, not p->next". I still do not get the idea to fix the segmentation fault, as I tried to change the algorithm, but the delete part will not work.. :(
Yes, about indent style, I thought it will be easier to read.