C++ - linked list - copy constructor

Question

I am trying to implement copy constructor and I have strange problem. Could you point what I am doing wrong?

I get: double free or corruption (out): 0x08936a48

Wrong output:

Constructor called...
MyList:
10 --> 20 --> 30 --> 40 --> 
MyList2:
10 --> 20 --> 30 --> 40 --> 
Destructor called...

Code:

#include <iostream>

template <typename T>
class SingleLinkedList{
protected:
    struct Node{
        T data;
        Node * next;
        Node() : next(nullptr) {}
        Node(const T num) : data(num), next(nullptr) {}
    };

private:
    Node * head;
    Node * tail;

public:
    SingleLinkedList();                                             // constructor
    ~SingleLinkedList();                                            // destructor
    SingleLinkedList(const SingleLinkedList &object);               // copy constructor
    SingleLinkedList& operator=(const SingleLinkedList &object);    // copy assignment

    void insert(T const& value);
    void displayList(std::ostream& stream = std::cout) const;

template <typename T>
SingleLinkedList<T>::SingleLinkedList() : head(nullptr), tail(nullptr){
    std::cout << "Constructor called..." << std::endl;
}

template <typename T>
SingleLinkedList<T>::~SingleLinkedList(){
    std::cout << "Destructor called..." << std::endl;
    int index = 1;
    Node * temp = nullptr;
    while(head!=nullptr){
        temp = head;
        head = head->next;
        delete temp;
        //std::cout << "Node number: " << index << " destroyed" << std::endl;
        index++;
    }
}

template <typename T>
SingleLinkedList<T>::SingleLinkedList(const SingleLinkedList<T> &oldList){
    SingleLinkedList<T> * newList = new SingleLinkedList<T>();

    // is it necessary? my constructor by default initializes head and tail with nulls
    head = nullptr;
    tail = nullptr;

    Node * temp = nullptr;
    temp = oldList.head;

    while(temp!=nullptr){
        newList->insert(temp->data);
        temp = temp->next;
    }
}


template <typename T>
void SingleLinkedList<T>::insert(T const& value){
    Node * temp = new Node(value);
    //temp->data = value;
    //temp->next = nullptr;


    if(head==nullptr){
        head = temp;
        tail = temp;
    }
    else{
        tail->next = temp;
        tail = temp;
    }
}

template <typename T>
void SingleLinkedList<T>::displayList(std::ostream& stream) const{
    Node * temp = nullptr;
    temp = head;

    while(temp!=nullptr){
        stream << temp->data << " --> ";
        temp = temp->next;
    }
    stream << std::endl;
}

int main(){

    SingleLinkedList<int> * myList = new SingleLinkedList<int>();
    SingleLinkedList<int> * myList2 = myList;

    myList->insert(10);
    myList->insert(20);
    myList->insert(30);

    myList2->insert(40);

    std::cout << "MyList:" << std::endl;
    myList->displayList();

    std::cout << "MyList2:" << std::endl;
    myList2->displayList();

    delete myList;
    delete myList2;

    return 0;
}

My "algorithm": 1. Create new list. 2. For each node of old list get data and insert it to new list.

I don't understand how using two different list I am deallocating the same part of memory. I am a student, not pro. I am asking for your understanding.

The copy constructor should initialize this. It's incorrect to new an new instance. Here you create a newList, fill it with elements and then discard/leak it. — François Andrieux
– François Andrieux, Commented May 18, 2018 at 13:30
Copy constructor works similar to normal constructor. Your object is already here, you don't create it with new. But this should not cause segfault here, because you copy empty object (so the loop is never entered). — Yksisarvinen
– Yksisarvinen, Commented May 18, 2018 at 13:34

bipll · Accepted Answer · 2018-05-18 13:39:17Z

2

Your copy ctor is really strange. Instead of adding elements to the list being constructed, you create a new list (why?) and then add elements to it. After that, you simply exit function. The new list created is left on the heap unreachable, i.e. leaked, and the list constructed remains empty.

Can you simply insert copied list's elements, not in a some newList, but in this one?

Double deletion happens for another reason: in your main you declare two pointers, myList and myList2, that both point to the same list in memory, and later try to delete them both. You can quickfix that by properly constructing myList2:

    SingleLinkedList<int> * myList2{new SingleLinkedList<int>(*myList)};

but I suggest you get rid of pointers at all in your main:

    SingleLinkedList<int> myList;
    SingleLinkedList<int> myList2(myList);

(And don't forget to change all ->s into .s below afterwards.)

This is not Java after all, not every lifetime starts with new.

answered May 18, 2018 at 13:39

bipll

12k1 gold badge21 silver badges33 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

pb. Over a year ago

Thank you so much for good explanation. Of course this creation of new object inside a function was unnecessary, I misunderstood some concept.

bipll Over a year ago

Hope now you understand it better. :)

Collectives™ on Stack Overflow

C++ - linked list - copy constructor

1 Answer 1

2 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

2 Comments

Your Answer

Sign up or log in

Post as a guest

Related