linked list - basic concept.

Hello, I started this week to deal a little bit with linked lists, I did a simple example, I'm not sure the way I did it is correct, maybe I'm missing something, here's the code:

Code:

---

#include <stdio.h>
#include <stdlib.h>

struct node {
        int data;
        struct node *next; //wohhaaa... a linked list :)
};

void add( struct node **head, int data ) {
        struct node *tmp = malloc( sizeof ( struct node ) );
       
        tmp->data = data;
        tmp->next = *head;

        *head = tmp;
}

void print( struct node *head ) {
        struct node *current = head;
       
        while( current ) {
                printf( "data: %d\n", current->data );
                current = current->next;
        }
}

void collect( struct node *head ) {
        struct node *current = head;
       
        while(current ) {
                free( current );
                current = current->next;
        }
}

int main( void ) {
        struct node *head = NULL;

        add( &head, 4 );
        add( &head, 3 );
        add( &head, 2 );

        print( head );
        collect( head );
        return 0;
}

---

My big question is, my collect() function really works? it's correct, or I'm forgetting to free something?

Quote:

---

my collect() function really works?

---

Nope, it's buggy and very dangerous. Why?

Code:

---

free( current );
current = current->next;

---

What makes you think that you can access a pointer that's already been freed? Just because it was freed the line before doesn't mean the memory hasn't been reused. A better way is to save the next pointer, then free the current pointer, then set the current pointer to the saved next pointer

Code:

---

void collect( struct node *head ) {
    struct node *current = head;
    struct node *forward;

    while(current ) {
        forward = current->next;
        free( current );
        current = forward;
    }
}

---

no its not correct. look

Code:

---

void collect( struct node *head ) {
        struct node *current = head;
       
        while(current ) {
                free( current ); // current is dead after this call
                current = current->next;// but you access it here.
        }
}

---

oops beaten

Ok, thanks for helping :)
Any other comments about my code? I'm still missing something?

I have altered your collect function, you should find the nodes are deleted correctly now

Code:

---


void collect( struct node *head ) {
        struct node *current = head;
        struct node *p;


        while(current){
                p = current->next;
                free( current );
                current = p;

                //check if node really is freed
                printf("\n\n Deleting node\n");
                print(current);

        }
}

---