747197 - Move number-conversion methods into NumericConversions.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: 1 - Move the simple conversion methods into a new header

Slims down jsnum.h, means code doesn't have to drag in the whole boatload of dependencies jsnum.h has.

I was tempted to also move the Value->integer methods, but unfortunately that fattens the dependencies a bit.  Probably that's the right thing to do, but it wasn't necessary to do it now, for my purposes, and it keeps the patch smaller.

The XDR changes, if I understand the comment about how we write/read doubles currently, shouldn't change how things get serialized.  As a precautionary measure I'm perfectly happy to bump the XDR version anyway, just to be safe.

A few places want to be able to access the bit patterns of doubles, but not to do mathematical operations on them.  We could keep doing it this way, with punning unions -- one-off as here, or in one central place as before (although I think that encourages people to do that even when they shouldn't, so I'm leery of offering it too prominently).  Another possibility is to do what WebKit does:

/*
 * C++'s idea of a reinterpret_cast lacks sufficient cojones.
 */
template<typename TO, typename FROM>
inline TO bitwise_cast(FROM from)
{
    COMPILE_ASSERT(sizeof(TO) == sizeof(FROM), WTF_bitwise_cast_sizeof_casted_types_is_equal);
    union {
        FROM from;
        TO to;
    } u;
    u.from = from;
    return u.to;
}

Perhaps disturbingly, I'm actually starting to find this idea pretty appealing.  It's too generic a facility for people to be likely to overuse it very often.  But it does get rid of the union-sprawl problem.  Maybe in a followup?  I think we'd want to put it in mfbt, not in the JS engine.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 2 - s/TIMECLIP/TimeClip/g

Now that TIMECLIP's no longer a macro, it's misnamed.  Moreover, the spec called it "TimeClip" from the start, so that name's definitely the right one.  (I didn't change it in the previous patch so I could keep it smaller.)

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 616764 [details] [diff] [review]
1 - Move the simple conversion methods into a new header

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

::: js/src/jsnum.cpp
@@ +89,4 @@
>  
>  #include "vm/MethodGuard-inl.h"
>  #include "vm/NumberObject-inl.h"
> +#include "vm/NumericConversions.h"

http://www.youtube.com/watch?v=9XJJwAZgAA0

Comment 3

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0405d42629fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/afab1aaf6704

I also landed this:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7b13e02cd5

which in a thinko I tagged as being a followup to these patches, but really it's a followup to changes made in bug 739380.  :-\  I'll note it there as well, just to be complete.

Comment 4

[Ed Morley [:emorley]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #3)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0405d42629fd
> https://hg.mozilla.org/integration/mozilla-inbound/rev/afab1aaf6704

Push backed out for win debug bustage, which persisted even after the original partial backout:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=718ced982de1

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdb6904fa2cf

Comment 5

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0405d42629fd

Comment 6

[(no longer active)]

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

Comment 7

[(no longer active)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/cdb6904fa2cf

Comment 8

[Jeff Walden [:Waldo]]

Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff687d407573
https://hg.mozilla.org/integration/mozilla-inbound/rev/11ea96115432

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Fix js1_8_5/extensions/clone-forge.js

Why we apparently have NO OTHER TESTS FOR THIS CASE (!!!), I don't know.

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 618876 [details] [diff] [review]
Fix js1_8_5/extensions/clone-forge.js

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

Awesome!

Comment 11

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7803bad0b1

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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