Add mozilla::SpecificNaNBits and use it in JS::GenericNaN and some other functions

RESOLVED FIXED in Firefox 52

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: arai, Assigned: arai)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Comment 1

a year ago
Things to do here:
  * Add mozilla::SpecificNaNBits and use create CanonicalizedNaNBits on global
  * Use CanonicalizedNaNBits in JS::GenericNaN
  * Make it possible to detect canonicalized NaN without creating the double JS::Value
Summary: Cleanup NaN handling in Value.h → Add mozilla::SpecificNaNBits and use it in JS::GenericNaN and some other functions
(Assignee)

Comment 2

a year ago
Created attachment 8803689 [details] [diff] [review]
Part 1: Add mozilla::SpecificNaNBits and JS::detail::CanonicalizedNaNBits.

Applied the comment in bug 1304191.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Attachment #8803689 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

a year ago
Created attachment 8803690 [details] [diff] [review]
Part 2: Add JS::IsCanonicalized and remove JS::Value::setDoubleNoCheck.
Attachment #8803690 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

a year ago
Created attachment 8803691 [details] [diff] [review]
Part 3: Do not check NaN in NumberValue(uint32_t i).

it won't need NaN handling, so we can just call Value::fromDouble, and it doesn't ivalidate constexpr.
(actually I'm not sure why constexpr is set only to this overload tho)
Attachment #8803691 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

a year ago
btw, what's the point of adding IntegerConstantValue separated from Int32Value or NumberValue? (bug 1304191 comment #41)
can't we just make NumberValue constexpr?
Attachment #8803689 - Flags: review?(jwalden+bmo) → review+
Attachment #8803690 - Flags: review?(jwalden+bmo) → review+
Attachment #8803691 - Flags: review?(jwalden+bmo) → review+
(In reply to Tooru Fujisawa [:arai] from comment #5)
> can't we just make NumberValue constexpr?

We could.  And maybe we should, because why not?

> what's the point of adding IntegerConstantValue separated from
> Int32Value or NumberValue? (bug 1304191 comment #41)

But because NumberValue has that double version, not *every* overload is guaranteed to be constant.  So in code that *demands* constexpr, using NumberValue is a bit of a footgun, because you don't *know* that using it will give constexpr behavior.  Maybe your expression's type will be changed at some point, and then suddenly you have a new static initializer.

Wherefore, JS::IntegerConstantValue that *only* has constexpr overloads.

This is possibly a little paranoid-ish, but can you imagine WebIDL codegen wanting to take the static-initializer risk if we can guarantee it'll always be eliminated?  I'd think they'd prefer definite safety.
(Assignee)

Comment 7

a year ago
Thank you for reviewing!

Filed bug 1313651 and bug 1313652, for NumberValue and IntegerConstantValue.
See Also: → bug 1313651, bug 1313652
(Assignee)

Comment 8

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8357a5b2431329993612ecd1095bff70f1d1911
Bug 1311088 - Part 1: Add mozilla::SpecificNaNBits and JS::detail::CanonicalizedNaNBits. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/9581faa40dcaf5b090e12d581efc08deb839c8cd
Bug 1311088 - Part 2: Add JS::IsCanonicalized and remove JS::Value::setDoubleNoCheck. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b8b8480075a26f7caeb6d2f4d49779a9632aa7
Bug 1311088 - Part 3: Do not check NaN in NumberValue(uint32_t i). r=jwalden

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c8357a5b2431
https://hg.mozilla.org/mozilla-central/rev/9581faa40dca
https://hg.mozilla.org/mozilla-central/rev/c2b8b8480075
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.