Correct way to write move constructor with unique_ptr member (crash)

Question

The following code will crash under Visual Studio 2013

I'm wondering why : what's the correct way to write a move constructor in such a case ? Removing the move constructor solves the problem. Is it a bug of VC++ or is this code wrong ?

What would be different in the default definition of the move constructor that makes this code not crash whereas my own definition does ?

#include <memory>
#include <vector>

class A
{};

class Foo
{
public:
    Foo(std::unique_ptr<A> ref) : mRef(std::move(ref)) {}
    Foo(Foo&& other) : mRef(std::move(other.mRef)) {}

    Foo(const Foo& other) {}
    Foo& operator=(const Foo& other) { return *this; }

protected:
    std::unique_ptr<A> mRef;
};

int main(int argc, char *argv[])
{
    std::vector<Foo>({ Foo(std::make_unique<A>()), Foo(std::make_unique<A>()) });
    // Crash : Debug Assertion Failed !
    // Expression : _BLOCK_TYPE_IS_VALID(pHead->nBlockUse)
}

It might be somehow related to this, right ?

Double delete in initializer_list vs 2013

Here is the actual code with fleshed out copy constructor and assignment, but the bug is exactly the same

class A
{
public:
     std::unique_ptr<A> clone() { return std::make_unique<A>(*this); }
};

class Foo
{
public:
    Foo(std::unique_ptr<A> ref) : mRef(std::move(ref)) {}
    Foo(Foo&& other) : mRef(std::move(other.mRef)) {}

    Foo(const Foo& other) : mRef(other.mRef->clone()) {}
    Foo& operator=(const Foo& other) { mRef = other.mRef->clone(); return *this; }

protected:
    std::unique_ptr<A> mRef;
};

My best guess is it's a bug that was solved in VS-2015, maybe see the link I added at the end ? — N0vember, Jan 24 '15 at 03:54
Probably unrelated to your current problem but anyway: Your *copy* constructor and assignment operator don't actually copy. — 5gon12eder, Jan 24 '15 at 03:58
@5gon12eder `unique_ptr` cannot be copied, so it is a requirement that either they don't copy or they are `delete`d. I think `delete`ing them (and supplying a move-assignment operator) would make more sense. `vector` does support more operations when it has a copy-constructor and copy-assignment operator available, however in this case those operations will yield results that don't make sense (e.g. resizing the vector could deallocate all the pointed-to objects). — M.M, Jan 24 '15 at 04:05
@MattMcNabb the actual code has a `clone` method in `class A`, but I removed it when tracking down the problem as it still caused a crash — N0vember, Jan 24 '15 at 04:08
@MattMcNabb Yes, the copy constructor and operator should either clone the managed object or be `delete`d. But defining them as no-ops is almost certainly wrong. @N0vember I suggest you remove them from your question if they are not required to reproduce your problem. — 5gon12eder, Jan 24 '15 at 04:10
@5gon12eder Well I believe removing them prevents the example from compiling, but I added another sample where they are not no-ops, still producing the same crash — N0vember, Jan 24 '15 at 04:24
is `clone` a virtual function? Or is `A` truly not involved in an inheritance hierarchy? If not, it would be simpler to just give `A` a copy constructor. — Howard Hinnant, Jan 24 '15 at 04:28
@HowardHinnant Yes it is a virtual function, again an effect of reducing the example to isolate the bug, your nitpick is valid — N0vember, Jan 24 '15 at 11:39

Howard Hinnant · Accepted Answer · 2015-01-24T04:51:14.487

This sounds like a VS-2013 bug. But it also looks like your code, though well-formed, is probably not doing what you want it too (however only you can say that for sure).

I added a print statement to your Foo:

class Foo
{
public:
    Foo(std::unique_ptr<A> ref) : mRef(std::move(ref)) {}
    Foo(Foo&& other) : mRef(std::move(other.mRef)) {}

    Foo(const Foo& other) {}
    Foo& operator=(const Foo& other) { return *this; }

    friend std::ostream&
    operator<<(std::ostream& os, const Foo& f)
    {
        if (f.mRef)
            os << *f.mRef;
        else
            os << "nullptr";
        return os;
    }

protected:
    std::unique_ptr<A> mRef;
};

And I added a print statement to your main as well:

aside: I also added identity/state to your A so that it was easier to see what is going on.

int main(int argc, char *argv[])
{
    std::vector<Foo> v({ Foo(std::make_unique<A>(1)), Foo(std::make_unique<A>(2)) });
    // Crash : Debug Assertion Failed !
    // Expression : _BLOCK_TYPE_IS_VALID(pHead->nBlockUse)
    for (const auto& p : v)
        std::cout << p << '\n';
}

For me this outputs:

nullptr
nullptr

which I believe is the correct output.

One can not move from an initializer_list, and thus the vector constructor invokes Foo's copy constructor which just default constructs unique_ptr.

Indeed if one removes the Foo copy constructor, which should be implicitly deleted then (or you could explicitly delete it), the program should not compile.

To really make this work, you have to give Foo an operational copy constructor. Perhaps something like this:

    Foo(const Foo& other)
        : mRef(other.mRef ? new A(*other.mRef) : nullptr)
    {}

So in summary, I think both the compiler and the current code get awards for bugs. Though from the comments, it sounds like the current code bug is simply an artifact of correctly reducing the code to isolate the problem.

VS-2013 bug.

As for your move constructor, it is fine. Though it would be even better if implemented with = default. If you do so, it will automatically inherit a noexcept spec. Such a spec should not be taken lightly. It is most important for efficient use of vector<Foo>.

My understanding is that VS-2013 does not understand either defaulted move members nor noexcept.

My anecdotal experience with VS-2013 is that the number of compiler bugs one experiences is directly proportional to the use of curly braces. I am hopeful that VS-2015 will contradict this experience. In the meantime I recommend avoiding using construction expressions that involve {}.

Update

Your updated copy members in:

class Foo
{
public:
    Foo(std::unique_ptr<A> ref) : mRef(std::move(ref)) {}
    Foo(Foo&& other) : mRef(std::move(other.mRef)) {}

    Foo(const Foo& other) : mRef(other.mRef->clone()) {}
    Foo& operator=(const Foo& other) { mRef = other.mRef->clone(); return *this; }

protected:
    std::unique_ptr<A> mRef;
};

have potential nullptr-dereference bugs. If other is in a moved-from state, the ->clone() is going to dereference a nullptr. Technically you can get away with this if you are very, very careful. However it will be very easy for you to accidentally hit this bug.

The solution to your update is just to check for nullptr or is it more complex ? — N0vember, Jan 24 '15 at 11:44
@N0vember: Right, just check for `nullptr`, and if found assign `nullptr` instead of calling `clone()`. I prefer using the conditional expression for this as shown in the example `Foo` copy constructor in my answer. But that's just a stylistic choice. — Howard Hinnant, Jan 24 '15 at 16:09

Correct way to write move constructor with unique_ptr member (crash)

1 Answers1