Closed Bug 885798 Opened 11 years ago Closed 10 years ago

Add new ES6 Number constants: EPSILON, MAX_SAFE_INTEGER, MIN_SAFE_INTEGER

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- fixed

People

(Reporter: bbenvie, Assigned: steveire)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [js:t][qa-][DocArea=JS])

Attachments

(3 files, 2 obsolete files)

From ES6 spec (May 2013 draft).

Number.EPSILON = 2.220446049250313e-16 (section 15.7.3.7)
Number.MAX_INTEGER = 9007199254740991 (section 15.7.3.8)
Sorry, EPSILON is 2.2204460492503130808472633361816e-16 (rounded a bit)
Attached patch Add new ES6 Number constants (obsolete) — Splinter Review
As described
Attachment #771708 - Flags: review?(jwalden+bmo)
Assignee: general → till
Attached patch Add new ES6 Number constants (obsolete) — Splinter Review
A qref later: tests!
Attachment #771762 - Flags: review?(jwalden+bmo)
Attachment #771708 - Attachment is obsolete: true
Attachment #771708 - Flags: review?(jwalden+bmo)
Comment on attachment 771762 [details] [diff] [review]
Add new ES6 Number constants

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

So, this is basically fine.  But I have concerns about the values being exposed here -- see latest es-discuss -- so I'd like you to hold off on landing this til those questions/concerns are resolved.

::: js/src/jsnum.cpp
@@ +1027,5 @@
>      {0,                         "MIN_VALUE",         0,{0,0,0}},
> +    /* ES6 (May 2013 draft) 15.7.3.7 */
> +    {2.2204460492503130808472633361816e-16, "EPSILON", 0,{0,0,0}},
> +    /* ES6 (May 2013 draft) 15.7.3.8 */
> +    {9007199254740991,          "MAX_INTEGER",       0,{0,0,0}},

I'd prefer (1ULL << 53) - 1 here.

::: js/src/tests/ecma_6/Number/15.7.3.7-EPSILON.js
@@ +11,5 @@
> + * BEGIN TEST *
> + **************/
> +
> +// Test value
> +assertEq(Number.EPSILON, 2.2204460492503130808472633361816e-16);

I'd prefer seeing Math.pow(2, -52) instead here.  (If there were a way to write that out in C++ as a constant expression, I'd want it there, too, but there isn't one.  :-( )

@@ +13,5 @@
> +
> +// Test value
> +assertEq(Number.EPSILON, 2.2204460492503130808472633361816e-16);
> +
> +// Test [[Writable]]: false

Seems better to me to test writability, enumerability, etc. using Object.getOwnPropertyDescriptor, rather than testing the observable effects of these.

::: js/src/tests/ecma_6/Number/15.7.3.8-MAX_INTEGER.js
@@ +11,5 @@
> + * BEGIN TEST *
> + **************/
> +
> +// Test value
> +assertEq(Number.MAX_INTEGER, 9007199254740991);

Math.pow(2, 53) - 1 as well here.  Same enumerability/configurability/writability test request too.
Attachment #771762 - Flags: review?(jwalden+bmo) → review+
Patch with review comments addressed. I'll wait for a final resolution of the discussion on es-discuss[1] before pushing this. It looks like this should be fine as-is, though.

[1]: https://mail.mozilla.org/pipermail/es-discuss/2013-July/031660.html
Attachment #771762 - Attachment is obsolete: true
I pushed a patch containing just EPSILON, for now, and will wait for an outcome of the es-discuss(ion).

https://hg.mozilla.org/integration/mozilla-inbound/rev/43744955a192
Status: NEW → ASSIGNED
Whiteboard: [leave open][js:t]
If I understand well, the Number.EPSILON will be in Firefox 25 (if not uplifted, backed out between now and the release). Am I correct?
>>I'd prefer seeing Math.pow(2, -52) instead here.  (If there were a way to write that out in C++ as a constant expression, I'd want it there, too, but there isn't one.  :-( )

nextafter(1) - 1

see http://www.cplusplus.com/reference/cmath/nextafter/
ah, it is not constant expr., sorry
Catching up with the latest tc39 meeting notes, I found this discussion of Number.MAX_INTEGER:

https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-07/july-25.md#59-semantics-and-bounds-of-numberisinteger-and-numbermax_integer

Personally, I find MM's war story fairly compelling. Waldo, what sayeth you?
Flags: needinfo?(jwalden+bmo)
I don't quite see how his story makes a difference one way or the other.  If the constant is 2**53, you can do < 2**53 to admit only "safe integers" that match his definition.  If the constant is 2**53 - 1, you can do <= for identical semantics.  It's all well and good to not consider 2**53 part of the "safe integer" set, but that doesn't say what constant should be exposed.

I'm having difficulty reading those notes.  Is

"""
MM:

MAX_SAFE_INTEGER = (2^53)-1

isInteger

    Infinity => false
    NaN => false
    value !== truncated value => false
    -0 => true

isSafeInteger

    -0 => true

toInteger

    Does not guarantee a safe integer

ToInteger

    Does not guarantee a safe integer
"""

the part where they actually decided anything specific?
Flags: needinfo?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> I don't quite see how his story makes a difference one way or the other.  If
> the constant is 2**53, you can do < 2**53 to admit only "safe integers" that
> match his definition.  If the constant is 2**53 - 1, you can do <= for
> identical semantics.  It's all well and good to not consider 2**53 part of
> the "safe integer" set, but that doesn't say what constant should be exposed.
> 
> I'm having difficulty reading those notes.  Is
> 
> """
> MM:
> 
> MAX_SAFE_INTEGER = (2^53)-1
> 
> isInteger
> 
>     Infinity => false
>     NaN => false
>     value !== truncated value => false
>     -0 => true
> 
> isSafeInteger
> 
>     -0 => true
> 
> toInteger
> 
>     Does not guarantee a safe integer
> 
> ToInteger
> 
>     Does not guarantee a safe integer
> """
> 
> the part where they actually decided anything specific?

I apologize for the lack of clear resolution in the minutes. I've posted to es-discuss: https://mail.mozilla.org/pipermail/es-discuss/2013-August/032990.html
Mark and Allen confirmed on that thread: 

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.7.2.8 
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.7.2.13 
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-15.7.2.14 


The following is copied from Mark's response for convenience:

> > Number.MAX_SAFE_INTEGER = (2^53)-1;
> 
> yes. Presumably
> 
> Number.MIN_SAFE_INTEGER = -Number.MAX_SAFE_INTEGER;
> 
> Number.isSafeInteger(n) {
>     return typeof n === 'number' && 
>         Math.round(n) === n && 
>         Number.MIN_SAFE_INTEGER <= n &&
>         n <= Number.MAX_SAFE_INTEGER;
> }
> 
> This has the important guarantee that
> 
> isSafeInteger(a) && isSafeInteger(b) && isSafeInteger(a+b) together imply that a+b is the > accurate sum of a and b.
> 
> Also
> 
> isSafeInteger(NaN) // false
> isSafeInteger(new Integer(2)) // false
> isSafeInteger(-0) // true
> isSafeInteger(Infinity) // false
> 
> We also decided that toInteger is not needed given the other similar coercion methods 
> already on Math.
Attachment #8413194 - Flags: review?(evilpies)
Comment on attachment 8413194 [details] [diff] [review]
Add Number.{MAX,MIN}_SAFE_INTEGER

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

Perfect. Thank you for your first contribution.
Attachment #8413194 - Flags: review?(evilpies) → review+
Whiteboard: [leave open][js:t] → [js:t]
Keywords: feature
Summary: Add new ES6 Number constants → Add new ES6 Number constants: EPSILON, MAX_INTEGER, MAX_SAFE_INTEGER, MIN_SAFE_INTEGER
This amends the previous commit from the bug report.

The lack of this extension happens not to matter because the
NC_EPSILON and NC_LIMIT enum values happen to not be used
in the file currently.
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0d7bf90091bb

Thanks again for the follow-up, and especially for seeing it in the first place!
Assignee: till → steveire
https://hg.mozilla.org/mozilla-central/rev/90515fc1d2cd
https://hg.mozilla.org/mozilla-central/rev/0d7bf90091bb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assuming this does not need QA before release. Please needinfo me if you think this needs testing.
Whiteboard: [js:t] → [js:t][qa-]
Whiteboard: [js:t][qa-] → [js:t][qa-][DocArea=JS]
MAX_INTEGER got never pushed (see comment 6) and is also not part of ES6. Changing the summary.
Summary: Add new ES6 Number constants: EPSILON, MAX_INTEGER, MAX_SAFE_INTEGER, MIN_SAFE_INTEGER → Add new ES6 Number constants: EPSILON, MAX_SAFE_INTEGER, MIN_SAFE_INTEGER
You need to log in before you can comment on or make changes to this bug.