921561 - JS_DECLARE_NEW_METHODS should use "perfect" forwarding

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: forward.diff

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 811252 [details] [diff] [review]
forward.diff

Why is the uint32_t cast needed?  I demand perfection!

Comment 2

[Jeff Walden [:Waldo]]

I think I mentioned this once on IRC, but you can't perfectly forward bit-field lvalues, because you can't form a reference to a bit-field.  Good times.  (This is in the commit message, I think.)

Comment 3

[Jeff Walden [:Waldo]]

So, this (or a slightly-tweaked version) busted on MSVC only.  I don't have the try link at hand for this any more.  :-\  My recollection is that MSVC seemed to be treating an rvalue as an lvalue, or vice versa, and that it might have been due to the extension note on <http://msdn.microsoft.com/en-us/library/bkbs2cds.aspx>.  But I'd have to page this all into memory to double-check, exactly.  Soon, but not now.

Comment 4

[Jeff Walden [:Waldo]]

Another try, that seems to work with MSVC10 locally, pending which I'll push this:

https://tbpl.mozilla.org/?tree=Try&rev=22fe07ab0c4b

Comment 5

[Jeff Walden [:Waldo]]

Once more, with feeling!

https://tbpl.mozilla.org/?tree=Try&rev=bd4e5eeb0b49

Comment 6

[Jim Blandy :jimb]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> I think I mentioned this once on IRC, but you can't perfectly forward
> bit-field lvalues, because you can't form a reference to a bit-field.  Good
> times.  (This is in the commit message, I think.)

This belongs in a comment in the code, not in the commit message. ("Why this temporary variable??")

Could we land the new_ forwarding bit separately? It's blocking bug 896100.

Comment 7

[Jim Blandy :jimb]

Attachment: Use mozilla::Forward in JS_DECLARE_NEW_METHODS new_ definitions.

This is the prequisite patch for bug 896100, extracted from one of Waldo's try pushes. I'm attaching it here so that I can link to it; the try push changeset will be cleaned up eventually, so a link to that would go dead.

Comment 8

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/474be320001f

Didn't see comment 6 before pushing, but given every compiler -- not just MSVC, or clang, or gcc, all of them -- will error instantly if you pass a bit field directly, it doesn't seem worth commenting on.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/474be320001f