Problems with removing the last item in a linked list in c

Question

I am just learning c and about linked lists I have some major problems.

I have following code :

#include <stdio.h>
#include <stdlib.h>
struct people {
    int age;
    char *name;
    struct people * next;
};
typedef struct people people;

void count(people array) {
    people *current=malloc(sizeof(people));
    current = &array;
    int count = 0;
    while(current){
        count++;
        printf("name %s\n",current->name);
        printf("age %d\n",current->age);
        current=current->next;
    }
    printf("%d\n", count);
    free(current);
}
void push(people *array){
    people * new=malloc(sizeof(people));
    people *last=malloc(sizeof(people));
    new->age=300;
    new->name="baz";
    new->next=NULL;
    last=array;
    while(last->next){
        last=last->next;
    }
    last->next=new;
//    free(new);
}
void pop(people *array){
    people * last=malloc(sizeof(people));
    last=array;
    while(last->next){
        //get the last element in the list
        last=last->next;
    }
//    free the last element 
    free(last);
}
int main(int argc, char** argv) {
    people person = {
        .name = "foo",
        .age = 25
    };
    person.next = malloc(sizeof (people));
    person.next->age = 26;
    person.next->name = "bar";
    person.next->next = NULL;
    //push into the list
    push(&person);
    //count after pushing
    count(person);
    //remove last
    pop(&person);
    //at this count i get just the age 0 but the name was not removed and still counts 3
    count(person);
    return 0;
}

When I run pop it is supposed to work similar to Array.prototype.pop from Javascript.
It behaves really weird the last next has the name "baz" and age 300. After I run this code instead of removing this last struct it just shows the age as 0.

Seems free is not really freeing the pointer allocated with malloc.

The one-before-last still points to invalid memory though. free() just returns the given memory chunk back to the memory allocator, doesn't set your pointers to valid memory — george_ptr, May 14 '16 at 08:39
In addition to @GeorgeAl comment you leak tons of memory. Current and last get their own memory, then you just drop it by assigning the pointer(s) other adresses. — Andreas, May 14 '16 at 08:44
@GeorgeAl so how do i free an allocated memory piece if free wont work ? — nikoss, May 14 '16 at 08:45
@nikoss, you need to set the `one-before-last->next = NULL;`. Also maybe try to explain to yourself why you do a `malloc()` at every possible chance. For example, why on `pop()` you `malloc()` at the first line? — george_ptr, May 14 '16 at 08:47
@Andreas i am coming from nodejs so whole this look after your memory stuff is really new to me the v8 engine did all those magic for me before :) — nikoss, May 14 '16 at 08:47
@GeorgeAl in order to make sure system can allocate a piece of memory for me i use malloc shouldnt i ? — nikoss, May 14 '16 at 08:48
@nikoss, you should allocate memory when you want to add a new `people` to your list, not when you want to remove one from your list. The memory (should be) already allocated by that time. — george_ptr, May 14 '16 at 08:49
@GeorgeAl i really dont get the point of malloc if life is better without it why to use it ? — nikoss, May 14 '16 at 08:49
@GeorgeAl doesn't just creating a variable allocate memory automatically why to use malloc then ? — nikoss, May 14 '16 at 08:50
@nikoss Ok. I advice you NOT learning c from scratch through coding and debugging. Pointers and memory is not that complex but you will have living hell reverse engineering how it works. — Andreas, May 14 '16 at 08:52
@nikoss, a local variable is rendered invalid on function exit `}` or early `return;`. Using an expired local variable (through its memory address) is Undefined Behaviour. You need memory that persists hence you use `malloc()`. Sorry but there is no point answering this question, you need to get a good `C` book. — george_ptr, May 14 '16 at 08:52
i removed all mallocs but still i don understand how it just makes age 0 and doesnt touch the rest i asign entire thing to null why it does not work — nikoss, May 14 '16 at 08:56
@GeorgeAl can you advice any book all i found is either rocket flying instructions or how to print something to stdout — nikoss, May 14 '16 at 08:58
@nikoss, See http://stackoverflow.com/questions/562303/the-definitive-c-book-guide-and-list — george_ptr, May 14 '16 at 08:59

score 0 · Answer 1 · answered May 14 '16 at 10:17

example

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

typedef struct people {
    int age;
    char *name;
    struct people * next;
} people;


people *new_people(const char *name, int age){
    people *node = malloc(sizeof(people));
    char *copy_name = malloc(strlen(name)+1);
    strcpy(copy_name, name);

    node->age = age;
    node->name = copy_name;
    node->next = NULL;
    return node;
}

void free_people(people *p){
    free(p->name);
    free(p);
}

void count(people *array) {
    people *current = array;
    int count = 0;

    while(current){
        count++;
        printf("name %s\n", current->name);
        printf("age %d\n",  current->age);
        current = current->next;
    }
    printf("%d\n", count);
}

void push(people **array, people *addNode){
    if(*array == NULL){
        *array = addNode;
        return ;
    }

    people *last = *array;
    while(last->next){
        last = last->next;
    }
    last->next = addNode;
    //return length;
}

people *pop(people **array){
    if(*array == NULL)
        return NULL;

    people *last = *array;
    people *prev = NULL;
    while(last->next){
        prev = last;
        last=last->next;
    }
    if(prev != NULL)
        prev->next = NULL;
    else
        *array = NULL;
    return last;
}

int main(void) {
    people *array = NULL;
    push(&array, new_people("foo",  25));
    push(&array, new_people("bar",  26));
    push(&array, new_people("baz", 300));

    count(array);
    people *baz = pop(&array);
    free_people(baz);
    count(array);

    people *bar = pop(&array);
    free_people(bar);
    people *foo = pop(&array);
    free_people(foo);//free_people(pop(&array))

    return 0;
}

CWLiu · Answer 2 · 2016-05-14T11:51:08.897

The problem is that, in the void count(people array), current=current->next; would be assigned in the while loop. So you need to make sure last->next shall be assigned to NULL in the pop function.

I modified your pop function to:

void pop(people *array){

    people * last=malloc(sizeof(people));

    while(array->next){
        last=array;
        array=array->next;
        if(array->next){
             //get the last element in the list
             last=last->next;
        }else{
             break;
        }                                                                                            
    }
    last->next=NULL;
    array=last;
}

In the pop function, you should assign the address of 'array' to 'last', and then points the 'array' to 'array->next'.

As the program breaks from the while loop, you can do last->next=NULL; and array=last; to make sure the last struct is fine.

Problems with removing the last item in a linked list in c

2 Answers2