892839 - Dubious example code in mfbt/Move.h

Status: RESOLVED INVALID

(Core :: MFBT, defect)

Description

[Justin Lebar (not reading bugmail)]

> One hint: if you're writing a move constructor where the type has members
> that should be moved themselves, it's much nicer to write this:
>
>   C(MoveRef<C> c) : x(c->x), y(c->y) { }
>
> than the equivalent:
>
>   C(MoveRef<C> c) { new(&x) X(c->x); new(&y) Y(c->y); }
>
> especially since GNU C++ fails to notice that this does indeed initialize x
> and y, which may matter if they're const.

Shouldn't the first line of code say

>   C(MoveRef<C> c) : x(Move(c->x)), y(Move(c->y)) { }

?

I'll write a patch, but wanted to make sure I was not going crazy...

Comment 1

[Jeff Walden [:Waldo]]

It might, somewhat, depend.  If initializing all the members that way results in the incoming MoveRef<T>'s object being in a destructible state, then things are good.  The other possibility is that MoveRef-constructing all the members, leaves the incoming object in an inconsistent state, that needs to be cleaned up in the constructor body.

Which is perhaps a longer way of saying the comment is not quite wrong, but not quite right.  I think.  I'm pretty sure it needs adjustment, but just smacking Move() around the member initializer expressions isn't really a correct fix.

Comment 2

[Jeff Walden [:Waldo]]

Oh, Move moved (har) into mfbt a day or two ago so that Vector can move there too (bug 891177), so moving this bug over there.

Move.

Comment 3

[Justin Lebar (not reading bugmail)]

Ah, I think I see what you're saying.

So the comment really should say something entirely different, indicating this gotcha.

Comment 4

[Luke Wagner [:luke]]

Comment 0 is right.  Move'ing the members should leave the members in a destructible state (by contract) and since the containing object will only be destroyed (again, by contract), this 

>   C(MoveRef<C> c) : x(Move(c->x)), y(Move(c->y)) { }

should be fine and indeed this is just the explicit form of what a C++11 compiler will generate for you with rvalue references.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 6

[Justin Lebar (not reading bugmail)]

Thanks for the quick review!

https://hg.mozilla.org/mozilla-central/rev/79439df5bf5d

Comment 7

[Luke Wagner [:luke]]

(In reply to Justin Lebar [:jlebar] from comment #6)
Whoa, spacetime is weakly ordered ;)

Comment 8

[Justin Lebar (not reading bugmail)]

Oh, lol; I saw your r+ in bug 893281 and pushed this one instead!

Comment 9

[Simon Giesecke [:sg] [he/him]]

mfbt/Move.h was removed by Bug 1609996, so this is obsolete now.