1302276 - Fix perfect forwarding for Zone::cellIter

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Steve Fink [:sfink] [:s:]]

Waldo quoth on IRC: "that's not how perfect forwarding is done..."

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fix perfect forwarding for Zone::cellIter

so I added some more punctuation. Uh... how about this?

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Don't abuse mozilla::Forward in move-constructor definitions to move members/base classes into the new object, and correctly use perfect forwarding in Zone::cellIter. NOT REVIEWED YET

I went and skimmed all the uses of mozilla::Forward in js/ and fixed them all, not just the one motivating this bug.

<Waldo> sfink: so, new rule: it's basically never correct to use mozilla::Forward<T>(t) when |t| is not an argument to the enclosing function, |T&&| is not the declared type of that argument, *or* |T| is not a template parameter type
<Waldo> sfink: and in particular, move constructors shouldn't use mozilla::Forward to forward stuff into member-initializers
<Waldo> sfink: this latter case you should just use mozilla::Move instead
<Waldo> sfink: mozilla::Forward basically only works in the intersection of all those cases I just mentioned, with basically only a single exception I can think of off the top of my head

sfink has blame on a few of these; hopefully he is now Enlightened.  Forwarding review to the other guilty party for a Teachable Moment.  :-)

There's one final use of mozilla::Forward in js/ that's a little dodgy, sort of.  Namely, gc/Marking.cpp's CallTraceHook.  It does the right things to perfectly forward.  BUT, it perfectly forwards a function parameter pack multiple times.  This is kosher, just a little bit strange in that it'll fail miserably if the forwarded type is move-only.  (The first forward will empty out the provided value into an argument to the called functor.  Then the second forward will forward a safely-destructible, but "empty", value.  That is, forwarding UniquePtr<int> would move that into the first call, then the second call would be passed a null UniquePtr.)  Because we're only forwarding an lvalue reference and a value, however, this doesn't actually matter.  But I thought I'd note it as something that reads slightly funny, while I'm here.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 8790509 [details] [diff] [review]
Don't abuse mozilla::Forward in move-constructor definitions to move members/base classes into the new object, and correctly use perfect forwarding in Zone::cellIter.  NOT REVIEWED YET

Review of attachment 8790509 [details] [diff] [review]:
-----------------------------------------------------------------

Okay!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc9f87959cd4
Don't abuse mozilla::Forward in move-constructor definitions to move members/base classes into the new object, and correctly use perfect forwarding in Zone::cellIter.  r=terrence

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc9f87959cd4

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc9f87959cd4