Move number-conversion methods into NumericConversions.h

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Created attachment 616764 [details] [diff] [review]
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.
Attachment #616764 - Flags: review?(luke)
Created attachment 616765 [details] [diff] [review]
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.)
Attachment #616765 - Flags: review?(luke)

Comment 2

5 years ago
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
Attachment #616764 - Flags: review?(luke) → review+

Updated

5 years ago
Attachment #616765 - Flags: review?(luke) → review+
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.
Target Milestone: --- → mozilla15

Comment 4

5 years ago
(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
Target Milestone: mozilla15 → ---
https://hg.mozilla.org/mozilla-central/rev/0405d42629fd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/afab1aaf6704
Backed out: https://hg.mozilla.org/mozilla-central/rev/cdb6904fa2cf
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff687d407573
https://hg.mozilla.org/integration/mozilla-inbound/rev/11ea96115432
Created attachment 618876 [details] [diff] [review]
Fix js1_8_5/extensions/clone-forge.js

Why we apparently have NO OTHER TESTS FOR THIS CASE (!!!), I don't know.
Attachment #618876 - Flags: review?(terrence)
Comment on attachment 618876 [details] [diff] [review]
Fix js1_8_5/extensions/clone-forge.js

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

Awesome!
Attachment #618876 - Flags: review?(terrence) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7803bad0b1
https://hg.mozilla.org/mozilla-central/rev/ef7803bad0b1
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.