Linked list using unique pointers won't run as I expected

Question

I'm trying to implement a linked list using unique pointers, something that I have done million times in c, but it seems that I can not make it work! The problem probably relies on the use of unique pointers but I'm not certain why.

In the following code you will notice two things. A class Node which contains the data of each element in the list and a class LinkedList which implements the behavior of the list.

#include <iostream>
#include <memory>

using namespace std;


class Node
{

//Private variables.
private:
    std::unique_ptr<Node> next; //Next node.
    std::unique_ptr<Node> prev; //Previous node.


    //Int value.
    int value;



//Public variables.
public:

    //Constructor.
    Node(int v)
    :next(nullptr), prev(nullptr), value(v)
    {
    }


    //Set next node.
    void set_next(std::unique_ptr<Node> new_node)
    {
       next = std::move(new_node);
    }


    //Set previous node.
    void set_prev(std::unique_ptr<Node> new_node)
    {
        prev = std::move(new_node);
    }


    //Set value.
    void set_value(int v)
    {
        value = v;
    }


    //Get next node.
    std::unique_ptr<Node> get_next()
    {
        return std::move(next);
    }


    //Get previous node.
    std::unique_ptr<Node> get_prev()
    {
        return std::move(prev);
    }


    //Get value.
    int get_value()
    {
        return value;
    }



};


class LinkedList
{

//Private variables.
private:
    std::unique_ptr<Node> head;
    std::unique_ptr<Node> tail;



//Public variables.
public:

    //Constructor.
    LinkedList()
    :head(nullptr), tail(nullptr)
    {
    }


    //Append a item to the list.
    void append(int v)
    {

        //Creating a new node.
        std::unique_ptr<Node> new_node( new Node(v) );


        //If this is the very first node.
        if (head == nullptr || tail == nullptr)
            {
                head = std::move(new_node);
                tail = std::move(new_node);
            }



        //Append.
        else
            {
                tail -> set_next( std::move(new_node) ); //Linking the new node.
                new_node -> set_prev( std::move(tail) ); //Set the previous.
                tail = std::move(new_node);              //Update the tail.
            }

    }


    //Print all the elements.
    void print()
    {

        //Starting node.
        std::unique_ptr<Node>curr = std::move(head);


        //While Loop.
        while(curr != nullptr)
            {
                cout << curr -> get_value() << endl;
                curr = std::move( curr -> get_next() );
            }
    }

};


int main()
{
LinkedList myList;

myList.append(1);
myList.append(2);
myList.append(3);
myList.append(4);

myList.print();

return 0;
}

Output should be 1,2,3,4 but instead it's only 4! I did debugging and i found out the following statement is running 4 times:

            //If this is the very first node.
        if (head == nullptr || tail == nullptr)
            {
                head = std::move(new_node);
                tail = std::move(new_node);
            }

But why? The first time, head and tail will be null but after that they will point somewhere so this statement should never run again.

Multiple unique pointers cannot point to the same object, and once a unique pointer has been moved to another unique pointer, the original no longer points to the object. — evan, Jul 14 '16 at 21:24
Step through the code in a debugger, and examine the pointers as you move them around to see that they don't match your expectations. — evan, Jul 14 '16 at 21:27
Answering a quick question posed in a comment to a deleted answer: Should I create data structures like this in C++? Answer 1: Rarely. The standard library already includes a list structure that does almost exactly this. Why reinvent the wheel? Answer 2: I'd do it a bit differently. If you make `Node` a private class inside `LinkedList` and never expose it to users you can save yourself a lot of effort you've devoted to protecting `node`. More here: http://stackoverflow.com/questions/38234616/linked-list-private-pointers-c/38235373#38235373 — user4581301, Jul 14 '16 at 22:02

score 4 · Accepted Answer · answered Jul 14 '16 at 21:29

4

One fundamental property of a std::unique_ptr is that only one std::unique_ptr can own a given pointer, at the same time. This happens to be what the "unique" part is all about.

std::unique_ptr<Node> new_node( new Node(v) );

Ok. So far so good.

   head = std::move(new_node);
   tail = std::move(new_node);

And that's your problem. You're trying to move the same std::unique_ptr into two others. That won't work. Only one of them can own a pointer.

Replace all your std::unique_ptrs with std::shared_ptrs.

answered Jul 14 '16 at 21:29

Sam Varshavchik

114,536
5
94
148

1

Recommend a slight tweak: Worth pointing out that `std::shared_ptr` eliminates the need for `std::move`. – user4581301 Jul 14 '16 at 21:46
Ok problem solved. I also want to ask: Should i create my data structures in c++ using smart pointers? In c i was handling the memory with the free() and malloc() functions. In c++ i found some posts saying is not good to use the 'new' keyword to store new objects into pointers and then delete them because of exception problems. – babaliaris Jul 14 '16 at 22:00
1

@babaliaris in C++ only use `malloc` and `free` in rare edge cases. Use `new` and `delete` instead because they know how to do constructors and destructors. Can using `new` and `delete` lead to problems? You betcha! Avoid where possible, but... A linked list is not a place where you can conveniently avoid it. Pretty much the point of having students write a linked list is to force them to learn to use pointers correctly and carefully. A shared pointer is not the right choice here. Shared pointer is when you have multiple competing owners of an object. Here the linked list is the owner. – user4581301 Jul 14 '16 at 22:10
`shared_ptr` works, but it's ideologically incorrect. `unique_ptr` also does not quite work because you any one node could be at the receiving end of a previous and a next. This is a job for raw pointers and a carefully written `LinkedList` class that makes certain that nodes are created and disposed of as required and never exposed to the `LinkedList`'s user. – user4581301 Jul 14 '16 at 22:13
i see... I know how to use pointers, `new` and `delete` to create data structures (is excactly the same with c where i used `malloc` and `free`). What i afraid of is that when you use `new` and `delete` you may have some leaks in memory because of exceptions like "out of memory" (At least thats what they said to me). How can i handle that? What more should i be aware of when using `new` and `delete`? Also if you know a good guide about this, a link would be nice :p – babaliaris Jul 15 '16 at 09:40
Εventually i found this link: [Dynamic Memory Allocation With new and delete](http://www.learncpp.com/cpp-tutorial/69-dynamic-memory-allocation-with-new-and-delete/) which was extremly usefull :p – babaliaris Jul 15 '16 at 11:04

Linked list using unique pointers won't run as I expected

1 Answers1