Open Bug 892839 Opened 6 years ago Updated 6 years ago

Dubious example code in mfbt/Move.h

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

People

(Reporter: justin.lebar+bug, Unassigned)

Details

Attachments

(1 file)

> 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...
Flags: needinfo?(luke)
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.
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.
Assignee: general → nobody
Component: JavaScript Engine → MFBT
Ah, I think I see what you're saying.

So the comment really should say something entirely different, indicating this gotcha.
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.
Flags: needinfo?(luke)
Attached patch Patch, v1Splinter Review
Attachment #775750 - Flags: review?(luke)
Summary: Dubious comment in Utility.h wrt MoveRef → Dubious example code in mfbt/Move.h
Attachment #775750 - Flags: review?(luke) → review+
(In reply to Justin Lebar [:jlebar] from comment #6)
Whoa, spacetime is weakly ordered ;)
Oh, lol; I saw your r+ in bug 893281 and pushed this one instead!
You need to log in before you can comment on or make changes to this bug.