unique_ptr, move constructor, and why always attempt to access private member

Question

I am encountering this problem frequently and I believe a move constructor is in order but I think the copy constructor is the problem and hiding it does not seem to work.

The code:

template <class T>
class LinkedList{
public:
    //
    LinkedList() {}
    LinkedList(const T &data);
    LinkedList(const T &data, const LinkedList &node);
    LinkedList(const LinkedList &object);
    LinkedList &operator=(const LinkedList &object);

    ~LinkedList() {}

    std::shared_ptr<LinkedList> push_back(const T& data);

private:
    T data;
    std::unique_ptr<LinkedList> link;

    std::unique_ptr<LinkedList> LinkFactory(const LinkedList &node);

    std::shared_ptr<LinkedList> CreateStartNode(const T &data);
    std::shared_ptr<LinkedList> CreateNode(const T &data, const LinkedList &node);
};

The particular line where the error is occurring is:

LinkedList<T>::LinkedList(const LinkedList<T> &object) : data(object.data),  
link(std::move(object.link)) {}

I am attempting to move, rather than copy, the link inside the copy constructor to no avail. If a move constructor is designed rather than synthesized, would that be better?

Since you have a copy constructor, the compiler will not synthesize a move constructor — David Rodríguez - dribeas, Feb 01 '13 at 15:36
If a move constructor is defined with a copy constructor, 'sometimes' the move constructor will be called. Is it then best to simply define and hide the implementation of a copy constructor? @dribeas If I don't define a copy constructor, one is synthesized for me and becomes a problem correct? — user633658, Feb 01 '13 at 15:48
Correct, you *need* your copy constructor. I am just saying that having a copy constructor inhibits the implicit declaration of the move constructor, so you need to declare and define it (even if the definition is plain `= default`, which in your case should work). That tackles your error message, but leaves the question of whether a copy constructor should steal the contents of the source open, well... not so much: it should not. — David Rodríguez - dribeas, Feb 01 '13 at 16:12
Unfortunately, I am using VS10 so `= delete` and `= default` are not supported. — user633658, Feb 01 '13 at 18:32

score 5 · Accepted Answer · answered Feb 01 '13 at 15:35

You can't move a constant object, and since object is declared const, object.link is const as well.

This looks like a broken design, since normally that constructor is a copy constructor, but you ary trying to move the link out of the parameter, meaning you try to steal it's owned resources. You have a LinkFactory method, looks like you should use that one if it does what the name promises.

Mike Seymour · Answer 2 · 2013-02-01T16:17:54.977

3

A constructor taking an lvalue reference is a copy constructor not a move constructor. If the reference is const then is can't modify the existing object, and so cannot move from it. (You should not remove the const, since that would give you a weird destructive-copy semantic, and move semantics were added to the language to avoid such weirdness).

Your class is not copyable due to the unique_ptr members, so you should not provide a copy constructor at all. You could provide a move constructor:

LinkedList<T>::LinkedList(LinkedList<T> && object) : 
    data(object.data), link(std::move(object.link)) {}

but there's no need to, since the implicitly generated move constructor does that (the only difference being that the data is moved rather than copied).

Remember that, usually, named variables can't be moved unless you do so explicitly:

LinkedList<int> l1;
LinkedList<int> l2(l1);             // ERROR: tries to copy
LinkedList<int> l3(std::move(l1));  // OK: explicit move

edited Feb 01 '13 at 16:17

answered Feb 01 '13 at 15:38

Mike Seymour

249,747
28
448
644

Thank you all. But if I fail to define a copy constructor then one is synthesized for me. With a move constructor defined, it may be called more frequently than the copy. How does this actually help resolve once and for all these issues? – user633658 Feb 01 '13 at 15:53
@user633658: Since your class contains uncopyable members, no copy constructor is synthesised; it's implicitly declared deleted. There is no sensible way to copy your class, so you should just leave it deleted. It *is* movable, and the implicit move constructor does the right thing, so there's no need to supply that either. Just don't try to copy instances of the class, since that won't work. – Mike Seymour Feb 01 '13 at 15:55
Is the implicit declaration of deleted `LinkedList(const LinkedList& object) = delete`? – user633658 Feb 01 '13 at 16:07
I just removed the constructor and indeed a synthesized one was made. `This diagnostic occurred in the compiler generated function 'LinkedList::LinkedList(const LinkedList &)'` I will place a move constructor into the class and it should be called in place of the copy. I also removed the operator assignment as well. – user633658 Feb 01 '13 at 16:12
@user633658: As I said, you shouldn't need to supply a move constructor since one is synthesised for you; but it will only be used when invoked with an *rvalue* (such as a temporary object, or one being returned from a function). To move a named variable, you'll need to use `std::move` to explicitly convert it to a suitable *rvalue*. – Mike Seymour Feb 01 '13 at 16:16
I set my copy constructor definition as such: `LinkedList::LinkedList(const LinkedList &other){ data = other.data; link = std::move(other.link); }` and still getting generated compiler error regarding the private variable in the unique_ptr class. I need more guidance and examples help. – user633658 Feb 01 '13 at 18:36
@user633658: As I said, you can't copy the `unique_ptr`, and you can't move it in a (sensible) copy constructor. If you really want your list to be copyable, then you'll need to do a deep copy: `link(new LinkedList(*(other.link)))` (or perhaps some more compilcated code to copy the list iteratively rather than recursively). I would recommend that you *don't* do this, since it's rarely sensible to copy large data structures; just leave the list movable but not copyable. – Mike Seymour Feb 02 '13 at 12:20

score 0 · Answer 3 · answered Feb 01 '13 at 15:38

0

What are the semantics you are trying to achieve? In your copy constructor, the object being copied is const (which is usually correct); trying to move anything in it would require it to be non-const. This looks like a design flaw, but if not, making the link mutable might be the answer.

answered Feb 01 '13 at 15:38

James Kanze

150,581
18
184
329

I am simply trying to achieve a means of copying an object but recognized that unique_ptr does not support copy/assign and tried to do that with 'std::move' instead. However, as pointed out, the const definition, though semantically correct, causes a problem two-fold. If I get rid of the copy and switch to a move, then I may have to deal with a synthesized which causes more problems. – user633658 Feb 01 '13 at 15:55
@user633658 I can see what you're trying to do at that level. But at a higher level. What should be the result of copying the object. Should it change the object being copied. (IIUC, your version would create a copy which hijakes all of the following objects in the list; i.e. which changes the object being copied.) – James Kanze Feb 01 '13 at 16:48

score 0 · Answer 4 · answered Feb 01 '13 at 19:21

I defined my copy constructor as follows and I have no compile-time or link errors using a unique_ptr:

LinkedList<T>::LinkedList(const LinkedList &other){
data = other.data;
link(std::move(other.link.get()));
}

I thank everyone for responding to this question.

unique_ptr, move constructor, and why always attempt to access private member

4 Answers4