Linked List insertion at the end

Question

I'm trying to insert a node at the end of the linked list but for some reason my print function tells me that the list is empty.

Struct:

struct node{
       int data;
       struct nodeList *next;
 };

Here is my addNode function

    struct node* addNode(struct node* List, int n){

    struct node* newNode = (struct node*)malloc(sizeof(struct node));
    struct node* temp = (struct node*)malloc(sizeof(struct node));


    newNode->data = n;
    newNode->next= NULL;

    if(List == NULL){
        List = newNode;
        return List;
    }

    temp = List;

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

    temp->next = newNode;
    return List;

    }

I appreciate any help!

I just forgot to type it in, it's in the code though. Thanks for the heads up — Kris, Apr 27 '16 at 21:41
Why are you allocating memory for `temp`? That's a memory leak. You're never using the storage that you allocate. — Tom Karzes, Apr 27 '16 at 21:43
In your caller, you need to update the caller's list pointer with the list head that this functions returns. You probably forgot to do so. — Tom Karzes, Apr 27 '16 at 21:45
@TomKarzes I thought I needed to allocate memory for temp because I use temp to iterate through the linked list — Kris, Apr 27 '16 at 21:46
Are you actually assigning the return value from `addNode` to the list head? — Dolda2000, Apr 27 '16 at 21:47
You use `temp`, yes. But you do not use what it points to. Look: You allocate memory. You assign it to `temp`. Then you assign `List` to `temp`. The memory you allocated is never used, and its address has been lost. That is a *severe* bug. You need to understand this *solidly* before attempting to use `malloc`. — Tom Karzes, Apr 27 '16 at 21:48
Here's what you need to do: (1) Fix the memory leak by deleting the call to `malloc` that is assigned to `temp`. (2) Fix the caller to update the caller's list head with the value this function returns. — Tom Karzes, Apr 27 '16 at 21:49
@TomKarzes That worked! I wasn't fixing the caller's list head with what the function returned. I feel stupid now for not realizing it lol, thank you for your help! I'm still trying to grasp LInked Lists. — Kris, Apr 27 '16 at 21:55
'I just forgot to type it in' - please don't do that again. Copy/paste in the source you are testing:) — Martin James, Apr 27 '16 at 23:57

score 2 · Accepted Answer · edited May 23 '17 at 10:28

2

You may be calling your function incorrectly, as others have noted.

struct node *numbers = NULL;
numbers = addNode(numbers, 5);

Alternatively, you could re-write your function to utilize double pointers. This would remove the need of having to re-assign your list pointer to the result of your addNode() function each time. It also shortens the function's implementation.

void addNode(struct node **list, int n) {

    struct node *newNode = malloc(sizeof(struct node));
    newNode->data = n;
    newNode->next= NULL;

    while (*list){
        list = &(*list)->next;
    }

    *list = newNode;
}

This can be called like so:

struct node *numbers = NULL;
addNode(&numbers, 4);

References:

edited May 23 '17 at 10:28

Community

1
1

answered Apr 27 '16 at 21:42

kylemart

1,156
1
13
25

1

There's nothing wrong with this. The new list head is being returned, which is what matters. True, there's no need to assign it to `List`, but doing so is harmless. – Tom Karzes Apr 27 '16 at 21:45
So when I return List, it doesn't return the newNode that I just set it to? – Kris Apr 27 '16 at 21:47
Fixed based on comment. – kylemart Apr 27 '16 at 21:50
@Kris It does return it, you just need to update the caller's copy. The alternative, which this post shows, is to have the caller pass the address of the list head pointer so that the function can update the caller's copy. Either method will work. The change shown here is an slternate way to do it, but is unnecessary. You can just fix the caller if you like. – Tom Karzes Apr 27 '16 at 21:51
1

The second fragment still leaks *temp. – wildplasser Apr 27 '16 at 22:00
Also: there is no need for the special case ( `if (! *list) {}` ) , And the extra parentheses are not needed. (see, for instance: http://stackoverflow.com/a/36824109/905902) – wildplasser Apr 27 '16 at 22:04
@wildplasser I've refactored the code. However, I feel as though this might be more confusing for the beginners. – kylemart Apr 27 '16 at 22:17
You don't need the extra parentheses. `=` has the lowest precedence (except for `,` ) -->> `List = &(*List)->next;` And the convention is (more or less) to use Initcaps for typenames – wildplasser Apr 27 '16 at 22:33
I'm aware of the convention. I was keeping consistent with the naming-style used by the OP. As for the parenthesis in `List = &(*List)->next;`, they cannot be removed. This is for the **exact** reason you specified: precedence. `List = &*List->next;` will not compile, nor would `List = &(*List->next);`. – kylemart Apr 27 '16 at 22:47
Note: you had an extra set of parentheses, a few edits ago. – wildplasser Apr 27 '16 at 23:47

Linked List insertion at the end

1 Answers1