c++ constructor seems to return pointer to the same object every time

Question

I have a simple implementation of a Linked list class, which has pointers to Node objects which I have also defined.

The function insertHead creates a new Node and inserts it at the head. However, whenever I call the function, the constructor seems to return a pointer to the same object every time. (I checked that using GDB)

I am pasting snippets of the code here. Can someone please let me know if something seems off?

void LinkedList::insertHead(int val){
  Node temp(val);
   if(head == NULL){
    head = tail = &temp;
  } else {
     temp.setNext(head);
     head = &temp;
  }
}

Class definitions:

class LinkedList{

  Node *head;
  Node *tail;
...

class Node{

  int val;
  Node *next;
...

Node::Node(int x){
  val = x;
  next = NULL;
}

`temp`, as you correctly named it, has automatic lifetime: you're storing dangling pointers to dead objects. — Quentin, Aug 10 '17 at 12:52
You're saving a pointer to a local variable that goes out of scope and becomes invalid at the end of that function. The pointer is a pointer to the stack, so it probably is the same every time. If you need to dynamically allocate objects, use `new` or some sort of unique or shared pointer. — Retired Ninja, Aug 10 '17 at 12:52

score 1 · Answer 1 · edited Aug 10 '17 at 13:11

You need to allocate your nodes on heap rather than stack. (I encourage you to read about those two). Also please use nullptr instead of NULL if supported ( > c++11)

void LinkedList::insertHead(int val){
  Node* temp = new Node(val);
  if(head == nullptr){
    head = tail = temp;
  } else {
     temp->setNext(head);
     head = temp;
  }
}

This will also require you clean up the nodes properly using the delete to avoid memory leaks which will most probably require adding a custom destructor to your list class, something along those lines:

LinkedList::~LinkedList() {
  Node* node = head;
  while(node != nullptr) {
    Node* toDel = node;
    node = node->next;
    delete toDel;
  }
}

Thanks. I didnt get that `Node temp(val)` will make an auto variable. That's what I was missing. — Hina Jajoo, Aug 11 '17 at 12:38

score 1 · Accepted Answer · answered Aug 10 '17 at 12:58

You cant assign address of automatic storage variable and use it out of the body, becouse it got out of scope (undefined behaviour). You need to dynamically allocate space on heap for the node.

Node * temp = new Node(val);
if(head == NULL)
{
    head = tail = temp;
} else {
    temp.setNext(head);
    head = temp;
}

And in destructor free all nodes.

c++ constructor seems to return pointer to the same object every time

2 Answers2