CODEWITHSUNDEEP
clinked-list
hocikto
hocikto

Reputation: 981

Deleting first node in linked list in C

I know that this was answered many times, but I just can't figure out what is wrong.

I have a structure

typedef struct zaznam{
   char kategoria[53];
   char znacka[53];
   char predajca[103];
   double cena;
   int rok;
   char stav[203];
   struct zaznam *next;
   }ZAZNAM;

And I want to delete first node this way.

for(prev = act = prvy; act!=NULL; prev=act, act=act->next){
    if((strstr(act->znacka,vyber)!=NULL)){

    if(act->znacka==prvy->znacka){ 
    //if "znacka" of the actual node is equal to the first

        prev=prvy->next; //in "previous" is pointer to the second node
        free((void *)act); //free first node
        prvy=prev; //first node = previous 
    }
    else{ //this works
        prev->next=act->next;
        free((void *)act);
        act=prev;
    }

And it works for everything but not for the first node.

Upvotes: 3

Views: 1615

Answers (3)

pm100
pm100

Reputation: 50190

why do you have a loop. Deleting the first entry in a list is just a matter of

tmp = head->next
delete head
head = tmp

Upvotes: 0

Jason
Jason

Reputation: 32510

Looks to me like when you delete the first node you're not properly assigning prev to point to the next node when you continue on the next iteration of the for-loop. For instance, in your first if statement to check if you're at the first node, you do the following:

prev=prvy->next; //in "previous" is pointer to the second node
free((void *)act); //free first node
prvy=prev; //first node = previous

That does free the first node, but, immediately in your for-loop you then assign act to prev here:

for(prev = act = prvy; act!=NULL; prev=act, act=act->next)

Since freeing memory doesn't actually erase the contents of memory, you proceed down the rest of the list as if the first node had not been freed. Actually using a freed pointer like that is undefined behavior, so anything could happen (i.e. you could crash, etc. ... in this case it seems like you aren't).

Try changing your code to the following:

for(prev = act = prvy; act!=NULL;)
{
    if((strstr(act->znacka,vyber)!=NULL))
    {
        if(act==prvy)
        { 
            //we're at the first node
            ZAZNAM* temp=act->next; 
            free((void *)act); //free first node
            prev=act=prvy=temp; //re-establish starting condition
        }
        else
        {   
            prev->next=act->next;
            free((void *)act);
            act=prev->next;
        }
    }
    else
    {
        //iterate here because if you delete the first node, you don't want
        //to start iterating like would happen if you kept iteration in the
        //for-loop declaration
        prev=act;
        act=act->next;
    }
}

Upvotes: 0

zkar
zkar

Reputation: 61

Well, you violate your own invariant prev->next == act already in the beginning: for (prev = act = prvy;... Second, this act->znacka==prvy->znacka should be act==prvy to find out whether you are at the beginning of the chain, otherwise it confuses people.

And I would probably try to reestablish you starting (but wrong) invariant (which is act==prev) by adding act=prev; for the first case. Maybe it will work.

Upvotes: 1

Related Questions