Why is my class node overwriting itself instead of making a new node object

Question

can someone run this code and tell me why the node in insert keeps overwriting?

#ifndef LinkedList_hpp
#define LinkedList_hpp

#include <stdio.h>
#include <utility>

template<class T>class LinkedList{
public:
LinkedList(){
        head = nullptr;
        tail = nullptr;
        size = 0;
    }
    //void insert(T val);
    class Node{
        public:
        Node* next;
        T* value;
        Node* prev;
        Node(T* value){
            this->value = value;
        }
        Node(){
        }
        Node(T* value,Node* prev, Node* next){
            this->value = value;
            this->next = next;
            this->prev = prev;
        }
        Node* operator=(const Node& node){
            this->value = node.value;
            this->prev = node.prev;
            this->next = node.next;
            return *this;
        }

    };
public:
    Node* head;
    Node* tail;
    int size;
    void insert(T val){

at this line, if the previous head was 10, the current val, 40, overwrites the old head value and inserts a new node with val 40

        Node* temp = new Node(&val);
        if(head==nullptr){
            head = temp;
            tail = temp;
        }else{
            temp->next = head;
            head->prev = temp;
            head = temp;
        }
        size++;
    }
#endif

#include <iostream>
#include "LinkedList.hpp"

int main(int argc, const char * argv[]) {

   // LinkedList<int> t;
    int h = 7;
    int j = 10;
    int k = 40;
    LinkedList<int>* list1 = new LinkedList<int>();


    list1->insert(h);
    list1->insert(j);
    list1->insert(k);

return 0;
}

every time insert is called and a new node is constructed, it overwrites the old value and everything becomes the current Val

score 2 · Answer 1 · answered Oct 05 '19 at 03:49

void insert(T val){

val is a parameter to this function. This object, this val, only exists until this function returns. At which point it gets destroyed, like everything else declared in non-static scope inside the function. That's how C++ works. Once insert() returns, this val no more. It ceases to exist. It goes to meet its maker. It becomes an ex-object, that no longer exist and is completely in the past.

Your insert() function does the following:

Node* temp = new Node(&val);

You're passing a pointer to this val parameter to Node's constructor, and Node then saves the pointer to a parameter to insert(), as its own class member.

That's great, but as soon as insert() returns, the saved pointer in the new-ed Node becomes a pointer to a destroyed object, and dereferencing this pointer becomes undefined behavior.

You are then, later, attempting to dereference the original pointer, which is no longer pointing to a valid object.

This explains the observed undefined behavior in your code.

The bottom line is that your classes and templates's design is fundamentally flawed. There is no apparent purpose for Node to use a pointer. Node should simply store T as its own class member, as the value, instead of value being a pointer to some other T which exists somewhere, and can get destroyed at any time, which is not under Node's control.

Another problem in the shown code is that two ofNode's constructor fail to initialize the next and prev pointers to NULL. This will also lead to undefined behavior.

THANKS! I fixed the issue by not making val a pointer. The way you explained it made a lot of sense. I was under the impression that the object pointed to maintained scope but I guess not. TY! — Drunkhydra, Oct 05 '19 at 04:08

score 1 · Answer 2 · edited Jun 20 '20 at 09:12

void insert(T val)

takes its arguments by value, so val is a local copy rather than the original.

Node* temp = new Node(&val);

stores a pointer to this local copy. The copy goes out of scope, so what you're looking at after insert exits is a ghost existing in memory that is no longer valid. In this case the ghost appears to always hold the last value set.

Solution:

Smart way: Store Node::value directly instead of as a pointer that you need to keep alive along with the node. Much less memory management this way.

T* value;

becomes

T value;

and

Node(T* value){
    this->value = value;
}

becomes

Node(T value){
        this->value = value;
}

The other uses of value must be updated accordingly. In general, new is such a headache that it should be used sparingly.

Stupid way: Pass by reference

void insert(T &val)

so that the pointer points at the longer lived original.

Why is my class node overwriting itself instead of making a new node object

2 Answers2

Solution: