Closed Bug 714260 Opened 9 years ago Closed 8 years ago

Provide double bit-twiddling macros in MFBT

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: Ms2ger, Assigned: Waldo)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

JSDOUBLE_IS_NaN / DOUBLE_IS_NaN and friends, in particular.
Duplicate of this bug: 720096
Attached patch Partial patch (obsolete) — Splinter Review
Putting off sleep, I decided to churn something out here.  This adds a header with inline methods for all the various double predicates (float predicates still need to be added), but it doesn't hook up any users yet.  That's a clear prerequisite to committing any sorts of tempting-looking implementations of such low-level queries.

This also needs DOUBLE_COMPARE at least added in.  But that macro currently has two versions, one for Windows and one for everything else, and as a result it requires specifying a value redundant on every platform but Windows.  I'd like to find out what still requires the Windows-specific version, because the extra argument used only on Windows seems like a great way to get something wrong in the future.  (I can't find a comment explaining why the macro bifurcates as it does, unfortunately.)  If nothing requires it, perhaps we could remove this macro entirely.
There's no way I'm going to remember the name of this file next time I need it ;)
Fair.  Suggest some other name, then (keeping in mind it'd have both double-predicates and float-predicates when complete).
Fair too :). Something about reals/floats/StdNonInt/? I'm bad at naming things.
I should probably take this so someone doesn't spend time updating my patch for me, as I expect I'll get this done in my spare time sometime soonish.
Assignee: nobody → jwalden+bmo
Attached patch Patch (obsolete) — Splinter Review
Ms2ger vaguely poked me about this today, so while I was waiting for jorendorff to show up to ask him a question, I polished off the bits of this I hadn't noodled into shape earlier.  dmandelin, feel free to just look at the js/src changes, I guess.

This doesn't remove all instances of jsdpun from js/src; as I recall (it's been awhile since I touched this other than to rebase it) there are uses for js_DoubleToECMAInt32.  But it's most of them, which is a good start.

I remember having posted previous iterations of this to try and getting back successful results.  I haven't done that with this yet, but I'll make sure to do so before pushing, should this pass reviews.
Attachment #590671 - Attachment is obsolete: true
Attachment #614208 - Flags: review?(dmandelin)
Attachment #614208 - Flags: review?(Ms2ger)
The NaN bit-pattern bits are a bit funky, to be sure.  Suggestions on that API appreciated.
Attachment #614208 - Flags: review?(dmandelin) → review+
Comment on attachment 614208 [details] [diff] [review]
Patch

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

Looks good to me, but I'd like glandium to have a look at the static initializers.

I haven't carefully reviewed the implementations in FloatingPoint.h; I assume you can copy+paste.

::: content/xslt/src/base/txDouble.cpp
@@ +53,5 @@
>  
>  //-- Initialize Double related constants
> +const double txDouble::NaN = MOZ_DOUBLE_NaN();
> +const double txDouble::POSITIVE_INFINITY = MOZ_DOUBLE_POSITIVE_INFINITY();
> +const double txDouble::NEGATIVE_INFINITY = MOZ_DOUBLE_NEGATIVE_INFINITY();

Will this cause static initializers to run? See bug 569629.

Actually, we could probably call the MOZ_* functions where we currently use the txDouble::* constants.

::: mfbt/FloatingPoint.h
@@ +57,5 @@
> +MOZ_STATIC_ASSERT((MOZ_DOUBLE_EXPONENT_BITS & MOZ_DOUBLE_SIGNIFICAND_BITS) == 0,
> +                  "exponent bits don't overlap significand bits");
> +
> +MOZ_STATIC_ASSERT((MOZ_DOUBLE_SIGN_BIT | MOZ_DOUBLE_EXPONENT_BITS | MOZ_DOUBLE_SIGNIFICAND_BITS)
> +                  == (uint64_t)(-1),

UINT64_MAX?

@@ +117,5 @@
> +}
> +
> +/**
> + * Determines whether a double is negative.  It is an error to call this method
> + * on a double which is NaN.

I assume you checked this for the callers you added?

@@ +237,5 @@
> +{
> +  /*
> +   * XXX Casting a double that doesn't truncate to int32_t, to int32_t, induces
> +   *     undefined behavior.  We should definitely fix this, but as apparently
> +   *     it "works" in practice, it's not a pressing concern now.

Could you file a bug and mention it here?
Attachment #614208 - Flags: review?(mh+mozilla)
Attachment #614208 - Flags: review?(Ms2ger)
Attachment #614208 - Flags: review+
Comment on attachment 614208 [details] [diff] [review]
Patch

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

::: content/xslt/src/base/txDouble.cpp
@@ +53,5 @@
>  
>  //-- Initialize Double related constants
> +const double txDouble::NaN = MOZ_DOUBLE_NaN();
> +const double txDouble::POSITIVE_INFINITY = MOZ_DOUBLE_POSITIVE_INFINITY();
> +const double txDouble::NEGATIVE_INFINITY = MOZ_DOUBLE_NEGATIVE_INFINITY();

Most likely, it will add static initializers. I do agree with Ms2ger, there is no need to keep these consts, now.
Attachment #614208 - Flags: review?(mh+mozilla) → review-
Attached patch Patch, v2Splinter Review
This hasn't fixed the double-int32_t casting comment yet to mention a bug.  I'll make sure to file one and update the comment before pushing this.
Attachment #614208 - Attachment is obsolete: true
Attachment #614480 - Flags: review?(Ms2ger)
Comment on attachment 614480 [details] [diff] [review]
Patch, v2

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

Ship it
Attachment #614480 - Flags: review?(Ms2ger) → review+
After a bit of tryservering (including of PGO builds, fun fun fun), this appears good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2187cab0d2f6

I'll get a blog post about this out sometime in the next week or so, also update MDN as needed for it.
Status: NEW → ASSIGNED
Keywords: dev-doc-needed
Target Milestone: --- → mozilla14
Blocks: 745089
https://hg.mozilla.org/mozilla-central/rev/2187cab0d2f6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 746273
Blog post:

http://whereswalden.com/2012/04/19/introducing-mozillafloatingpoint-h-methods-for-floating-point-types-and-values/

I also updated https://developer.mozilla.org/En/mfbt to minimally point to the header.  With those, I guess we're done on the documentation front.
Turns out MOZ_HASH_DOUBLE, modeled on JSDOUBLE_HASH or whatever, became unused sometime between when I first wrote this patch and when I landed it.  Also there were some extraneous #includes, for the same reason.  After a double-check on try I removed these bits here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f44ffa3f9ff
Blocks: 788108
You need to log in before you can comment on or make changes to this bug.