Fix perfect forwarding for Zone::cellIter

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript: GC
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sfink, Assigned: Waldo)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Waldo quoth on IRC: "that's not how perfect forwarding is done..."
(Reporter)

Comment 1

2 years ago
Created attachment 8790501 [details] [diff] [review]
Fix perfect forwarding for Zone::cellIter

so I added some more punctuation. Uh... how about this?
Attachment #8790501 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

2 years ago
Created 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

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.
Attachment #8790509 - Flags: review?(terrence)
(Assignee)

Updated

2 years ago
Assignee: sphink → jwalden+bmo
(Assignee)

Updated

2 years ago
Attachment #8790501 - Attachment is obsolete: true
Attachment #8790501 - Flags: review?(jwalden+bmo)
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!
Attachment #8790509 - Flags: review?(terrence) → review+

Comment 4

2 years ago
Pushed by jwalden@mit.edu:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc9f87959cd4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.