C Simple Linked List - Wrong output

Question

I'm printing out the Head and Tail of the Linked List when an element is added, Fairly simple.

int main(){

    struct node{
        struct node* next;
        struct node* previous;
        double value;
    };

    struct LinkedList{
        struct node* head;
        struct node* tail;
    };

    void addValue(struct LinkedList* list,double newValue){
        struct node newNode;
        newNode.next = NULL;
        newNode.value=newValue;
        if(list->head == NULL){
            newNode.previous=NULL;
            list->head= &newNode;
            list->tail=&newNode;


        }
        else
        {
            newNode.previous= list->tail;
            list->tail->next= &newNode;
            list->tail= &newNode;



        }
            printf("%f\n",list->head->value);
            printf("%f\n",list->tail->value);

    }

    struct LinkedList l1;
    l1.head=NULL;
    l1.tail=NULL;
    addValue(&l1,5);
    addValue(&l1,6);
    addValue(&l1,7);
    addValue(&l1,8);


}

But the output I get is

5.000000 5.000000 6.000000 6.000000 7.000000 7.000000 8.000000 8.000000

Instead what I expect

5.000000 5.000000 5.000000 6.000000 5.000000 7.000000 5.000000 8.000000

Any idea why?

you should create new nodes on the heap with the malloc command instead of on the stack as currently is done. — catchmeifyoutry
– catchmeifyoutry, Commented Jan 28, 2014 at 19:22
@catchmeifyoutry Good catch, it looks like dangling pointers. — user1508519
– user1508519, Commented Jan 28, 2014 at 19:23
@remyabel you can try to write my catch in an answer if you want, I have some other stuff to do ;) — catchmeifyoutry
– catchmeifyoutry, Commented Jan 28, 2014 at 19:25

score 4 · Accepted Answer · 2014-01-28 19:36:57Z

4

As stated in the comments, you should create newNode on the heap. When the function exits, the pointers are no longer pointing to valid memory. Luckily, the changes you need to make are minimal.

    struct node* newNode = (struct node*)malloc(sizeof(struct node));
    newNode->next = NULL;
    newNode->value=newValue;
    if(list->head == NULL){
        newNode->previous=NULL;
        list->head= newNode;
        list->tail=newNode;
    }
    else
    {
        newNode->previous= list->tail;
        list->tail->next= newNode;
        list->tail=newNode;
    }

And to avoid memory leaks you should free your pointers. Here's an example implementation:

void deleteList(struct node** head_ref)
{
   struct node* current = *head_ref;
   struct node* next;

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

   *head_ref = NULL;
}
deleteList(&l1.head);

I tested it in valgrind and that should eliminate your leaks.

edited Jan 28, 2014 at 19:36

answered Jan 28, 2014 at 19:25

user1508519

Sign up to request clarification or add additional context in comments.

2 Comments

ItayB Over a year ago

And don't forget to implenent delete function so the memory will be released at the end

catchmeifyoutry Over a year ago

maybe you can mention that the memory should be freed too, to complete the implementation

Sekhar Pedamallu · Accepted Answer · 2014-01-29 08:37:21Z

0

By defining struct node newNode; in void addValue(struct LinkedList* list,double newValue), memory gets allocated on the stack. so when control from the function returns, no more access to stack. values are gone !!!!. Thats its required to allocate from heap, dynamic memory where it cna be accesses until it is freed.

answered Jan 29, 2014 at 8:37

Sekhar Pedamallu

1

Collectives™ on Stack Overflow

C Simple Linked List - Wrong output

2 Answers 2

2 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

2 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related