-2

I am having difficulties with my C program function. The functions idea is to add an item (newval) to end of the linked list. Here is my code (also find the comments with it):

#include <stdlib.h>

struct list
{

    int val;
    struct list *next; 
};

void add_to_list(struct list *l, int newval)

{

    if (!l) return;  //my fried said that I should add this, but I dont really understand why.
                     //and should it be if(l == NULL) rather than if(!l)? 
    while(l->next !=NULL) // I am pretty sure that this while loop is correct
       l=l->next; 
    l->next = malloc(sizeof(int)) //hmm... is this correct way to allocate memory
    l->next->val = newval;  //not sure about this
    l->next->next = NULL; //This should be fine
}
Nerd Bergg
  • 1
  • `l->next = malloc(sizeof(int))` That is indeed the correct way to allocate memory, but you're not allocating enough. You need to allocate enough the contain an entire `struct list`. Change this line to `l->next = malloc(sizeof(*(l->next)))` – Daniel Kleinstein Dec 03 '14 at 17:06

3 Answers3

2
l->next = malloc(sizeof(int));

You need to allocate memory to your structure not for int

l->next = malloc(sizeof(struct list));

if(!l) is same as if(l == NULL)

Gopi
  • 19,784
  • 4
  • 24
  • 36
  • Is if(!l) really the same as if(l == NULL). I thought if(!l) means if(l == 0)? – Nerd Bergg Dec 03 '14 at 18:17
  • @NerdBergg NULL is 0.Check this http://stackoverflow.com/questions/1296843/what-is-the-difference-between-null-0-and-0 – Gopi Dec 03 '14 at 18:18
  • Ok, this is clear, but then again, why do weed this if sentence here? I mean, dosent this check, that if the list is empty, then return. But why to return, if the list is empty, cant we still add there a item newval? Or does it mean, that if the list is empty, we can not add anything to end of it... we should first add the first item to it and this would have to be done in a different function? – Nerd Bergg Dec 03 '14 at 19:13
  • It doesn't check whether the list is empty, it checks whether you've been passed a valid list. If the `add_to_list()` function is not passed a valid list, there isn't any mechanism present to allow you to create and return one, so your only option is to exit while doing nothing. – This isn't my real name Dec 03 '14 at 22:10
0

you should allocate

l->next = malloc(sizeof(struct list));

instead of 

l->next = malloc(sizeof(int));

and initialize the allocated memory with zero:

memset(l->next, 0, sizeof(struct list));
chrmue
  • 1,552
  • 2
  • 18
  • 35
  • I don't agree. Why should `memset()` be used to initialize the structure? This isn't nearly a large enough chunk of memory to warrant that. – This isn't my real name Dec 03 '14 at 22:47
  • OK, I didn't see the line l->next->next = NULL -> memset is redundant in this case. I just wanted to asure that everything is initialized properly. As far as I know memset works fine for the smallest possible size of a memory chunk (1 Byte). – chrmue Dec 04 '14 at 08:12
0
    void add_to_list(struct list *l, int newval)

Your function is declared as void. This means there's no way for your function to signal an error. Not a problem for sample code, but definitely something you should at least be aware of.

    if (!l) return;  //my fried said that I should add this,
                     //and should it be if(l == NULL) rather than if(!l)?

The construct (!l) is completely equivalent to (l != NULL). This is because NULL is defined to be "a null pointer constant", and "null pointer constant" is "An integer constant expression with the value 0, or such an expression cast to type void *". In other words, NULL evaluates to zero.

    while(l->next !=NULL) // I am pretty sure that this while loop is correct
        l=l->next;

Yes, this while loop is correct. When the loop completes, l will be pointing at the last element in the list.

    l->next = malloc(sizeof(int)) //hmm... is this correct way to allocate memory

Your allocation is almost right. You're allocating enough storage to hold one int, but you really need enough storage to hold one struct list. use l->next = malloc(sizeof(struct list)); instead.

    l->next->val = newval;  //not sure about this

No error here. This is exactly as correct as the next line is.

    l->next->next = NULL; //This should be fine

Yes, this is fine..

However, while this section of code is mostly correct, barring the malloc() that was already covered,

    l->next = malloc(sizeof(int)) //hmm... is this correct way to allocate memory
    l->next->val = newval;  //not sure about this
    l->next->next = NULL; //This should be fine

the method used here is slightly unusual. This is because when you allocate the new memory, you're assigning the pointer directly to l->next and then initializing it.

What is more commonly done is todeclare a pointer such as struct list *tmp = NULL at the beginning of the function, and when allocating, assign the new memory to tmp. Then, initialize the new list element pointed at by tmp, and only then add the now-initialized new list element to the end of the list. While this involves slightly more code, it separates the two steps of creating the new list element and of adding the element to the end of the list.

    void add_to_list(struct list *l, int newval)
    {
        struct list *tmp = NULL;

        if (!l) return;  // If no valid list, return.

        tmp = malloc(sizeof(struct list)); // Allocate new list element
        tmp->val = newval;                 // and fill it up.
        tmp->next = NULL;   // End of the list always points nowhere.

        while(l->next !=NULL) // find end of list.
            l=l->next;

        l->next = tmp; // Attach new list element to end.

        return;
    }

Also, doing it this way avoids the potential confusion that can arise from assigning values through the l->next pointer.

This isn't my real name
  • 4,869
  • 3
  • 17
  • 30