Nodes added to linked list revert back to NULL after function

Question

I'm reading lines from a file and storing them into linked list.

void add(llist *list, somevalue) {
    Node *newnode = (Node *) malloc(sizeof(Node));
    newnode->value = somevalue;
    newnode->next = list->head;
    list->head = newnode;
}

and I call this function from an initialize function which opens the file and reads lines from the file.

void init() {
    llist *list = (llist *) malloc(sizeof(llist));
    //
    //bunch of file i/o codes
    //
    while (read file until it returns NULL) {
        add(list, line);
        //if I try to print the values of the list here it works.
    }
    //Outside the loop, the head is back to NULL

}

And another problem that I realized is the values get concatenated every time I try to print the value. That is to say, the output would be:

First Loop: Tony
Second Loop: Peter
             TonyPeter
Third Loop: Mir
            PeterMir
            TonyPeterMir

1. How do I fix it so the add function permanently adds the node to the linked list?

2. Why would the values be jumbled up like that?

----EDITED----

The list is a global variable, and here are some more snippets from the init function. This is the while loop with the problem:

//open file
//initialize & declare pointers
while (1) {
            for (i = 0; i < max; i++) {
                value[i] = '\0';
            }
            if (!(fgets(value,max,f))) {
                //segfaults if I try to print out the list inside this block.
                break;
            }

            add(list, value);

            //the values are well separated in this statement
            printf("id is %s\n", list->head->value);

            //This print list works, but works weird as shown above.
            print_list(list);
    }
    fclose(f);

    //This print list doesn't work, the list is NULL
    print_list(list);

And this is the print list function:

void print_users(llist *list) {
    ListNode *e;
    if (list->head == NULL) {
            printf("NO USERS\r\n");
            return;
    }
    e = list->head;
    while (e != NULL) {
            puts(e->id);
            e = e->next;
    }

}

Answer 1

So I don't have a good grasp at all on what you're exactly trying to do here, so we can only do but so much. You may consider posting a MCVE. However, I may be able to give you some pointers on building a linked list. I directly copied your add function into a linked list class that I just hurriedly built, and it worked fine, so there may be something else in your llist class that is causing the issue, or it could be something else in your code. The class and a brief description of the class are listed below.

basic node class Note: I used templates, but you could just as easily remove the template statements and replace T with any type.

template<typename T>
class node {
    private:
        T       data;
        node*   next;

    public:
        // The C++11 rule of 5
        // Default Constructor
        node(T value = T(), node* nxt = nullptr) : data(value), next(nxt) { }

        // Copy Constructor
        node(const node<T>& src) : data(src.data), next(src.next) { }

        // Move Constructor
        node(node<T>&& src) : data(src.data), next(src.next) {
            src.data = T();
            src.next = nullptr;
        }

        // Copy Assignment Operator
        node<T>& operator=(const node<T>& src) {
            this->data = src.data;
            this->next = src.next;
            return(*this);
        }

        // Move Assignment Operator
        node<T>& operator=(node<T>&& src) {
            this->data = src.data;
            this->next = src.next;
            src.data = T();
            src.next = nullptr;
        }

        // Destructor
        ~node() {};

        // Some functions to help with encapsulation

        void set_next(node* nxt) {
            this->next = nxt;
        }

        void set_data(T value) {
            this->data = value;
        }

        node* get_next() {
            return(this->next);
        }

        T& get_data() {
            return(data);
        }
};

linked list class body Since you're using dynamic memory, you need to make sure you adhere to the rule of 3 or 5 depending on whether or not you're using C++11.

template<typename T>
class llist {
    private:
        node<T>* head;

    public:
        llist();

        llist(const llist& src);

        llist(llist&& src);

        llist(~llist);

        llist& operator=(const llist& src);

        llist& operator=(llist&& src);

        void push();

        void insert();
};

default constructor Nothing fancy here.

template<typename T>
llist<T>::llist() : head(nullptr) { }

copy constructor Since you're using dynamic memory this is crucial

template<typename T>
llist<T>::llist(const llist& src) {
        node<T>* tmp = src.head;
        while(tmp) {
            this->push(tmp->get_data());
        }
}

move constructor

template<typename T>
llist<T>::llist(llist&& src) {
    // delete current nodes
    node<T>* tmp = this->head;
    while(tmp) {
        tmp = head->get_next();
        delete head;
        head = tmp;
    }
    // steal the sources list
    this->head = src.head;
    src.head = nullptr;
}

destructor

template<typename T>
llist<T>::~llist() {
    node<T>* tmp = this->head;
    while(tmp) {
        tmp = head->get_next();
        delete head;
        head = tmp;
    }
}

copy assignment operator

template<typename T>
llist& llist<T>::operator=(const llist<T>& src) {
    node<T>* tmp = src.head;
    while(tmp) {
        this->push(tmp->get_data());
    }
    return(*this);
}

move assignment operator

template<typename T>
llist& llist<T>::operator=(llist<T>&& src) {
    node<T>* tmp = this->head;
    while(tmp) {
        tmp = head->get_next();
        delete head;
        head = tmp;
    }
    this->head = src.head;
    src.head = nullptr;
    return(*this);
}

push member this is essentially opposite of your add member.

template<typename T>
void llist<T>push(T data) {
    node<T>* new_node = new node<T>(data);
    if(this->head) {
        node<T>* tmp = this->head;
        while(tmp->get_next()) {
            tmp = tmp->get_next();
        }
        tmp->set_next(new_node);
    } else {
        this->head = new_node;
    }
}

insert member This is essentially your add member.

template<typename T>
void llist<T>insert(T data) {
    node<T>* new_node = new node<T>(data, this->head);
    this->head = new_node;
}

I don't know if this will help, and you probably already have and know most of this, but I hope it helps.

Answer 2

In this code, it would appear that you attempted to 'malloc' space for a "llist" user defined object.

void init() {

    llist *list = (llist *) malloc(sizeof(llist));
    //
    //bunch of file i/o codes
    //
    while (read file until it returns NULL) {
        add(list, line);
        //if I try to print the values of the list here it works.
    }
    //Outside the loop, the head is back to NULL
}

First, you tagged this as C++. In C++, you simply must use new and delete. The C++ compiler does not associate "malloc" with the ctor / dtor of your user created object called "llist". And I assure you that you really do want to create these two methods, even when each are simple. Really.

On the other hand, the C++ compiler does provide New and Delete, and will automatically invoke the ctor and dtor when appropriate for both dynamic variables (in heap), and automatic variables (on stack). The compiler will not support this with malloc.

---

Second, your function init() does not return or otherwise deliver the value of the automatic variable you named "list" to any other scope. (Typically, a list lifetime exceeds the life of any function that uses or creates it.)

So your object "list" only exists within the scope of the function init(), within the lifetime of init(). Not very useful.

So the handle to the list of 'malloc'ed things is lost, no longer accessible to anything else. After init(), where did you plan for the listHead to reside?

Even if you used new (and delete) the code still does not deliver the listHead anywhere.

To further your program, you need perhaps 1 of 2 things:

1) a return (from the function) of the "list" handle (I've been calling it "listHead" as you intended, right?)

llist* init() {
    llist *listHead = ...

    return(listHead);
}

OR

2) a parameter reference which your init function changes. This places the list head outside of init().

void init( llist** listHead) {
    llist *list = ...

    *listHead = list;
}

You might look into, and take hints from std::list, which has 40+ methods, though you might only need 10. For the methods you plan to implement, you should strive to conform to and use similar names and parameters.

Perhaps you meant to use a class data attribute with the label list (it is quite difficult to imagine this from what you provided). In this case, you should distinguish data attributes names to help you remember what it is, and that it has a different scope. For instance, I would use m_listHead. The prefix m_ (or often, simply the one char prefix '_') simply indicates to the reader that this symbol is a data attribute of a class. This idea is a common c++ idiom, and not enforced by the compiler, but is often part of a coding-standard.

Good luck.