18

Synopsis

How can I safely design a move constructor when a class uses multiple inheritance?

Details

Consider the following scenario:

struct T { };
struct U { };

struct X : public T, public U
{
    X(X&& other)
      : T(std::move(other))
      , U(std::move(other)) // already moved?!
    {
    }
};

Is there a way to move-construct both T and U safely?

ildjarn
  • 62,044
  • 9
  • 127
  • 211
mavam
  • 12,242
  • 10
  • 53
  • 87

3 Answers3

18

tl;dr: the code in the question is ok.

The code above is fine, because std::move itself doesn't actually change other in any way, it just does a cast to make other into an rvalue reference so that the move constructors of T and U are called instead of their copy constructors.

When T(std::move(other)) is run, T's move constructor will be called (assuming it has one) and the T in other will be moved to the T in this. The U in other will be left alone until the U(std::move(other)) is run.

Note that this means that when your move constructor code for X runs, you cannot rely on the members/member functions of T and U in other, as those bits of other will have already have been moved.

As a side note, it could be improved by being changed to:

X(X&& other)
  : T(std::move(static_cast<T&>(other)))
  , U(std::move(static_cast<U&>(other)))
{
}

because this version doesn't rely on the implicit upcast from X&& to T&&/U&&. Relying on the implicit upcast can be a problem because T and/or U may have a T(X&&) constructor or an accept-anything template constructor, either of which would get picked instead of the T(T&&) move constructor that you really want to call.

je4d
  • 7,628
  • 32
  • 46
  • 1
    until someone in calls a virtual function or something in a move constructor/assignment operator which is addressed by another class in inheritance tree... rihgt? –  Apr 11 '12 at 22:52
  • 3
    Why not `T(static_cast(other))` rather than `T(std::move(static_cast(other)))` (and the same for `U`)? – ildjarn Apr 11 '12 at 22:53
  • @ildjarn that would also work if `T` is a concrete class, but could be a problem if `T` was a template parameter because of reference collapsing. I was trying to avoid "inlining" std::move to make it clearer what I was changing – je4d Apr 11 '12 at 22:55
  • @VladLazarenko correct - so don't call virtual functions in your move constructors :-) – je4d Apr 11 '12 at 22:57
  • 3
    There are no templates in this example, and if there were you would probably want `std::forward` rather than `std::move`. I think using `move` here confuses things, personally, but I see what you're getting at. :-] – ildjarn Apr 11 '12 at 22:59
  • 1
    @ildjarn i'm not sure what you're getting at with `std::forward`, but then i'm not even sure whether you're thinking of a template constructor or a template class. My point about reference collapsing was a dud, because even if T and U were template args, they'd have to be real classes as they're inherited from. Still, since we are doing a move, I think it's clearer to do the explicit upcast and the move separately. It also encourages the use of `std::move` for doing lvalue-to-rvalue conversions over writing casts manually, which I think is a good thing. – je4d Apr 11 '12 at 23:12
  • You brought up reference collapsing, so it was assumed that you meant a template constructor. In any case, like I said, I see what you're getting at, I just think it's extra noise if there's a `static_cast<>` already. – ildjarn Apr 11 '12 at 23:14
  • @ildjarn yeah, I accidentally confused things by bringing up reference collapsing. Granted, if it was a template constructor you'd probably want `std::forward`. But since the original question was about move constructors, and I can't see any sane way for a move constructor to be a template, this is getting a bit offtopic :-) – je4d Apr 11 '12 at 23:24
1

Can I use other object in sidetone constructor body after using moving it like:

struct T { };
struct U { };

struct X : public T, public U
{
    X(X&& other)
      : T(std::move(other))
      , U(std::move(other)) // already moved?!
    {
    member1 = other.member1; //something like. 

    }
};
heenabansal
  • 51
  • 3
0

The code in the question is fine, as the answer from @je4d explained it.

You could also write it this way :

X(X&& other)
  : T(static_cast<T&&>(other))
  , U(static_cast<T&&>(other))
{
}

See the comment of the answer from @jead for explanation.