Unique Pointer attempting to reference a deleted function

1
votes

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

3
c++pointersunique-ptr
You can't copy unique_ptrs. Your print function can't work without moving the unique_ptr you use as a parameter, making it empty. I would make the parameter const node&. As print function doesn't keep any reference to the parameter after it returns, it doesn't need to know about how it is managed. - Neil Kirk
print is broken in other ways too -- right now it is an infinite loop. And insertNode needs to be pass-by-reference also - Ben Voigt
auto main() -> int. really? - szulak
You shouldn't really be using unique_ptr like this. For example, by passing a unique_ptr to your print function, you're implying that it is taking ownership of the pointer; when really that isn't the case. The enclosing scope of a unique_ptr is said to be the sole owner of it (which is why it's called unique); you can move this ownership but not share/copy it. A shared_ptr, on the other hand, represents ownership that is shared; a raw/weak pointer or reference (probably what your print function should accept... const qualified) generally indicates a lack of ownership. - Julian

3 Answers

4
votes

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.

0
votes

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.

0
votes

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.