Is the following move constructor code safe?

Question

This is the move constructor of class X:

X::X(X&& rhs)
    : base1(std::move(rhs))
    , base2(std::move(rhs))
    , mbr1(std::move(rhs.mbr1))
    , mbr2(std::move(rhs.mbr2))
{ }

These are the things I'm wary about:

1. We're moving from rhs twice and rhs isn't guaranteed to be in a valid state. Isn't that undefined behavior for the initialization of base2?
2. We're moving the corresponding members of rhs to mbr1 and mbr2 but since rhs was already moved from (and again, it's not guaranteed to be in a valid state) why should this work?

This isn't my code. I found it on a site. Is this move constructor safe? And if so, how?

Answer 1

No, that would be an example of possible undefined behavior. It might work a lot of the time, but it's not guaranteed to. C++ allows it, but you are the one who has to make sure you don't try to use rhs again in that way. You are trying to use rhs three times after its guts had been ripped out its rvalue reference was passed to a function that might potentially move from it (leaving it in an undefined state).

Answer 2

This is approximately how an implicit move constructor typically works: each base and member subobject is move-constructed from the corresponding subobject of rhs.

Assuming that base1 and base2 are bases of X that do not have constructors taking X / X& / X&& / const X&, it's safe as written. std::move(rhs) will implicitly convert to base1&& (respectively base2&&) when passed to the base class initializers.

EDIT: The assumption has actually bitten me a couple of times when I had a template constructor in a base class that exactly matched X&&. It would be safer (albeit incredibly pedantic) to perform the conversions explicitly:

X::X(X&& rhs)
    : base1(std::move(static_cast<base1&>(rhs)))
    , base2(std::move(static_cast<base2&>(rhs)))
    , mbr1(std::move(rhs.mbr1))
    , mbr2(std::move(rhs.mbr2))
{}

or even just:

X::X(X&& rhs)
    : base1(static_cast<base1&&>(rhs))
    , base2(static_cast<base2&&>(rhs))
    , mbr1(std::move(rhs.mbr1))
    , mbr2(std::move(rhs.mbr2))
{}

which I believe should exactly replicate what the compiler would generate implicitly for X(X&&) = default; if there are no other base classes or members than base1/base2/mbr1/mbr2.

EDIT AGAIN: C++11 §12.8/15 describes the exact structure of the implicit member-wise copy/move constructors.

Answer 3

It depends on the inheritance hierarchy. But chances are very good that this code is fine. Here is a complete demo showing that it is safe (for this specific demo):

#include <iostream>

struct base1
{
    base1() = default;
    base1(base1&&) {std::cout << "base1(base1&&)\n";}
};

struct base2
{
    base2() = default;
    base2(base2&&) {std::cout << "base2(base2&&)\n";}
};

struct br1
{
    br1() = default;
    br1(br1&&) {std::cout << "br1(br1&&)\n";}
};

struct br2
{
    br2() = default;
    br2(br2&&) {std::cout << "br2(br2&&)\n";}
};

struct X
    : public base1
    , public base2
{
    br1 mbr1;
    br2 mbr2;

public:
    X() = default;
    X(X&& rhs)
    : base1(std::move(rhs))
    , base2(std::move(rhs))
    , mbr1(std::move(rhs.mbr1))
    , mbr2(std::move(rhs.mbr2))
    { }
};

int
main()
{
    X x1;
    X x2 = std::move(x1);
}

which should output:

base1(base1&&)
base2(base2&&)
br1(br1&&)
br2(br2&&)

Here you see that each base and each member is moved exactly once.

---

Remember: std::move doesn't really move. It is just a cast to rvalue, nothing more.

---

So the code casts to rvalue X and then passes that down to the base classes. Assuming the base classes look like I have outlined above, then there is an implicit cast to rvalue base1 and base2, which will move construct those two separate bases.

Also,

---

Remember: A moved-from object is in a valid but unspecified state.

---

As long as the move constructor of base1 and base2 don't reach up into the derived class and alter mbr1 or mbr2, then those members are still in a known state and ready to be moved from. No problems.

Now I did mention that problems could occur. This is how:

#include <iostream>

struct base1
{
    base1() = default;
    base1(base1&& b)
         {std::cout << "base1(base1&&)\n";}
    template <class T>
    base1(T&& t)
        {std::cout << "move from X\n";}
};

struct base2
{
    base2() = default;
    base2(base2&& b)
        {std::cout << "base2(base2&&)\n";}
};

struct br1
{
    br1() = default;
    br1(br1&&) {std::cout << "br1(br1&&)\n";}
};

struct br2
{
    br2() = default;
    br2(br2&&) {std::cout << "br2(br2&&)\n";}
};

struct X
    : public base1
    , public base2
{
    br1 mbr1;
    br2 mbr2;

public:
    X() = default;
    X(X&& rhs)
    : base1(std::move(rhs))
    , base2(std::move(rhs))
    , mbr1(std::move(rhs.mbr1))
    , mbr2(std::move(rhs.mbr2))
    { }
};

int
main()
{
    X x1;
    X x2 = std::move(x1);
}

In this example, base1 has a templated constructor that takes an rvalue-something. If this constructor can bind to an rvalue X, and if this constructor will move from the rvalue X, then you have problems:

move from X
base2(base2&&)
br1(br1&&)
br2(br2&&)

The way to fix this problem (which is relatively rare, but not vanishingly rare), is to forward<base1>(rhs) instead of move(rhs):

    X(X&& rhs)
    : base1(std::forward<base1>(rhs))
    , base2(std::move(rhs))
    , mbr1(std::move(rhs.mbr1))
    , mbr2(std::move(rhs.mbr2))
    { }

Now base1 sees an rvalue base1 instead of an rvalue X, and that will bind to the base1 move constructor (assuming it exists), and so you again get:

base1(base1&&)
base2(base2&&)
br1(br1&&)
br2(br2&&)

And all again is good with the world.