Move number-conversion methods into NumericConversions.h

RESOLVED FIXED in mozilla15



JavaScript Engine
5 years ago
5 years ago


(Reporter: Waldo, Assigned: Waldo)



Firefox Tracking Flags

(Not tracked)



(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;

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


5 years ago
Attachment #616765 - Flags: review?(luke) → review+

I also landed this:

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)

Push backed out for win debug bustage, which persisted even after the original partial backout:
Target Milestone: mozilla15 → ---
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Backed out:
Resolution: FIXED → ---
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]:

Attachment #618876 - Flags: review?(terrence) → review+
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.