Last Comment Bug 747197 - Move number-conversion methods into NumericConversions.h
: Move number-conversion methods into NumericConversions.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks: 739380
  Show dependency treegraph
 
Reported: 2012-04-19 15:05 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-04-27 06:51 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1 - Move the simple conversion methods into a new header (34.86 KB, patch)
2012-04-19 15:05 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
2 - s/TIMECLIP/TimeClip/g (3.80 KB, patch)
2012-04-19 15:07 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Fix js1_8_5/extensions/clone-forge.js (1.75 KB, patch)
2012-04-26 17:39 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-19 15:05:55 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-19 15:07:13 PDT
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.)
Comment 2 Luke Wagner [:luke] 2012-04-19 15:45:08 PDT
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] (remove +bmo to email) 2012-04-24 16:39:54 PDT
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] 2012-04-25 01:59:43 PDT
(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 7 :Ehsan Akhgari 2012-04-25 07:47:58 PDT
Backed out: https://hg.mozilla.org/mozilla-central/rev/cdb6904fa2cf
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-26 17:39:30 PDT
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.
Comment 10 Terrence Cole [:terrence] 2012-04-26 17:44:57 PDT
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] (remove +bmo to email) 2012-04-26 18:34:32 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7803bad0b1
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-04-27 06:51:09 PDT
https://hg.mozilla.org/mozilla-central/rev/ef7803bad0b1

Note You need to log in before you can comment on or make changes to this bug.