714260 - Provide double bit-twiddling macros in MFBT

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

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

JSDOUBLE_IS_NaN / DOUBLE_IS_NaN and friends, in particular.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Partial patch

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.

Comment 3

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

There's no way I'm going to remember the name of this file next time I need it ;)

Comment 4

[Jeff Walden [:Waldo]]

Fair.  Suggest some other name, then (keeping in mind it'd have both double-predicates and float-predicates when complete).

Comment 5

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

Fair too :). Something about reals/floats/StdNonInt/? I'm bad at naming things.

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Patch

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.

Comment 8

[Jeff Walden [:Waldo]]

The NaN bit-pattern bits are a bit funky, to be sure.  Suggestions on that API appreciated.

Comment 9

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

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?

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Patch, v2

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.

Comment 12

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

Comment on attachment 614480 [details] [diff] [review]
Patch, v2

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

Ship it

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/2187cab0d2f6

Comment 15

[Jeff Walden [:Waldo]]

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.

Comment 16

[Jeff Walden [:Waldo]]

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

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4f44ffa3f9ff