The default bug view has changed. See this FAQ.

Provide double bit-twiddling macros in MFBT

RESOLVED FIXED in mozilla14

Status

()

Core
MFBT
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla14
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
JSDOUBLE_IS_NaN / DOUBLE_IS_NaN and friends, in particular.
(Reporter)

Updated

5 years ago
Duplicate of this bug: 720096
Created attachment 590671 [details] [diff] [review]
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.
(Reporter)

Comment 3

5 years ago
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).
(Reporter)

Comment 5

5 years ago
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
Created attachment 614208 [details] [diff] [review]
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.
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+
(Reporter)

Comment 9

5 years ago
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-
Created attachment 614480 [details] [diff] [review]
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.
Attachment #614208 - Attachment is obsolete: true
Attachment #614480 - Flags: review?(Ms2ger)
(Reporter)

Comment 12

5 years ago
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
Last Resolved: 5 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.
Keywords: dev-doc-needed → dev-doc-complete
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
https://hg.mozilla.org/mozilla-central/rev/4f44ffa3f9ff

Updated

5 years ago
Blocks: 788108
You need to log in before you can comment on or make changes to this bug.