Unique Pointer attempting to reference a deleted function

Question

Hello I am trying to use pointers and learning the basics on unique pointers in C++. Below is my code I have commented the line of code in main function. to debug the problem However, I am unable to do so. What am I missing ? Is my move() in the insertNode() incorrect ? The error I get is below the code :

#include<memory>
#include<iostream>

struct node{
    int data;
    std::unique_ptr<node> next;
};


void print(std::unique_ptr<node>head){
        while (head)
            std::cout << head->data<<std::endl;
    }


std::unique_ptr<node> insertNode(std::unique_ptr<node>head, int value){
        node newNode;
        newNode.data = value;
        //head is empty
        if (!head){
            return std::make_unique<node>(newNode);
        }
        else{
            //head points to an existing list
            newNode.next = move(head->next);
            return std::make_unique<node>(newNode);
        }
    }



auto main() -> int
{
    //std::unique_ptr<node>head;
    //for (int i = 1; i < 10; i++){
    //  //head = insertNode(head, i);
    //}
}

ERROR std::unique_ptr>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function

Answer 1

Aside from other small problems, the main issue is this line:

return std::make_unique<node>(newNode);

You are trying to construct a unique pointer to a new node, passing newNode to the copy constructor of node. However, the copy constructor of node is deleted, since node contains a non-copyable type (i.e. std::unique_ptr<node>).

You should pass a std::move(newNode) instead, but this is problematic since you create the node on the stack and it will be destroyed at the exit from the function.

Using a std::unique_ptr here is a bad idea in my opinion, since, for example, to print the list (or insert into the list), you need to std::move the head (so you lose it) and so on. I think you're much better off with a std::shared_ptr.

Answer 2

I was having the same problem and indeed using a shared_ptr works.

Using the smart pointer as an argument in the function copies the pointer (not the data it points to), and this causes the unique_ptr to reset and delete the data it was previously pointing at- hence we get that "attempting to reference a deleted function" error. If you use a shared_ptr this will simply increment the reference count and de-increment it once you are out of the scope of that function.

The comments in the answers above suggest that using a shared_ptr is baseless. These answers were written before the C++17 standard and it is my understanding that we should be using the most updated versions of the language, hence the shared_ptr is appropriate here.

Answer 3

I don't know why we have to expose node type to user in any case. Whole thingamajig of C++ is to write more code in order to write less code later, as one of my tutors said.

We would like to encapsulate everything and leave no head or tail (pun intended) of node to user. Very simplistic interface would look like:

struct list
{
private:
  struct node {
      int data;
      std::unique_ptr<node> next;
      node(int data) : data{data}, next{nullptr} {}
  }; 
  std::unique_ptr<node>  head;
public:   
  list() : head{nullptr} {};

  void push(int data);
  int pop();

  ~list(); // do we need this?
};

The implementation does something what Ben Voigt mentioned:

void list::push(int data) 
{
    auto temp{std::make_unique<node>(data)};
    if(head) 
    {
        temp->next = std::move(head);
        head = std::move(temp);
    } else 
    {
        head = std::move(temp);
    }
}

int list::pop() 
{
   if(head == nullptr) {
      return 0; /* Return some default. */
      /* Or do unthinkable things to user. Throw things at him or throw exception. */
   }  
   auto temp = std::move(head);
   head = std::move(temp->next);
   return temp->data;
}  

We actually need a destructor which would NOT be recursive if list will be really large. Our stack may explode because node's destructor would call unique_ptr's destructor then would call managed node's destructor, which would call unique_ptr's destructor... ad nauseatum.

void list::clear() { while(head) head = std::move(head->next); }

list::~list() { clear(); }

After that default destructor would ping unique_ptr destructor only once for head, no recursive iterations.

If we want to iterate through list without popping node, we'd use get() within some method designed to address that task.

Node *head = list.head.get();
/* ... */
head = head->next.get();

get() return raw pointer without breaking management.

Answer 4

How about this example, in addition to the sample code, he also mentioned some principles:

when you need to "assign" -- use std::move and when you need to just traverse, use get()