Last Comment Bug 885798 - Add new ES6 Number constants: EPSILON, MAX_SAFE_INTEGER, MIN_SAFE_INTEGER
: Add new ES6 Number constants: EPSILON, MAX_SAFE_INTEGER, MIN_SAFE_INTEGER
Status: RESOLVED FIXED
[js:t][qa-][DocArea=JS]
: dev-doc-complete, feature
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla31
Assigned To: Stephen Kelly
:
:
Mentors:
Depends on:
Blocks: es6
  Show dependency treegraph
 
Reported: 2013-06-21 10:28 PDT by Brandon Benvie [:benvie]
Modified: 2014-06-05 14:48 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Add new ES6 Number constants (1.57 KB, patch)
2013-07-05 15:30 PDT, Till Schneidereit [:till]
no flags Details | Diff | Splinter Review
Add new ES6 Number constants (3.75 KB, patch)
2013-07-06 03:27 PDT, Till Schneidereit [:till]
jwalden+bmo: review+
Details | Diff | Splinter Review
Add ES6 Number constants EPSILON and MAX_INTEGER. (3.52 KB, patch)
2013-07-11 07:38 PDT, Till Schneidereit [:till]
no flags Details | Diff | Splinter Review
Add Number.{MAX,MIN}_SAFE_INTEGER (3.22 KB, patch)
2014-04-26 07:39 PDT, Stephen Kelly
evilpies: review+
Details | Diff | Splinter Review
Extend the nc_slots enum with new values (1.06 KB, patch)
2014-04-26 09:48 PDT, Stephen Kelly
no flags Details | Diff | Splinter Review

Description Brandon Benvie [:benvie] 2013-06-21 10:28:29 PDT
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)
Comment 1 Brandon Benvie [:benvie] 2013-06-21 10:30:17 PDT
Sorry, EPSILON is 2.2204460492503130808472633361816e-16 (rounded a bit)
Comment 2 Till Schneidereit [:till] 2013-07-05 15:30:20 PDT
Created attachment 771708 [details] [diff] [review]
Add new ES6 Number constants

As described
Comment 3 Till Schneidereit [:till] 2013-07-06 03:27:31 PDT
Created attachment 771762 [details] [diff] [review]
Add new ES6 Number constants

A qref later: tests!
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-09 13:24:04 PDT
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.
Comment 5 Till Schneidereit [:till] 2013-07-11 07:38:56 PDT
Created attachment 773980 [details] [diff] [review]
Add ES6 Number constants EPSILON and MAX_INTEGER.

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
Comment 6 Till Schneidereit [:till] 2013-07-13 06:47:08 PDT
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
Comment 7 Ed Morley [:emorley] 2013-07-15 02:57:21 PDT
https://hg.mozilla.org/mozilla-central/rev/43744955a192
Comment 8 Jean-Yves Perrier [:teoli] 2013-08-05 14:15:24 PDT
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?
Comment 9 4esn0k 2013-08-07 07:42:59 PDT
>>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/
Comment 10 4esn0k 2013-08-07 07:44:06 PDT
ah, it is not constant expr., sorry
Comment 11 Till Schneidereit [:till] 2013-08-07 16:55:52 PDT
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?
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-12 18:39:31 PDT
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?
Comment 14 Rick Waldron [:rwaldron] 2013-08-29 11:03:14 PDT
(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
Comment 15 Rick Waldron [:rwaldron] 2013-08-29 14:11:03 PDT
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.
Comment 16 Stephen Kelly 2014-04-26 07:39:56 PDT
Created attachment 8413194 [details] [diff] [review]
Add Number.{MAX,MIN}_SAFE_INTEGER
Comment 17 AWAY Tom Schuster [:evilpie] 2014-04-26 07:52:38 PDT
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.
Comment 18 Till Schneidereit [:till] 2014-04-26 08:01:50 PDT
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/90515fc1d2cd

Thanks for the contribution! \o/
Comment 19 Stephen Kelly 2014-04-26 09:48:06 PDT
Created attachment 8413221 [details] [diff] [review]
Extend the nc_slots enum with new values

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.
Comment 20 Till Schneidereit [:till] 2014-04-26 09:56:46 PDT
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!
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2014-05-01 14:48:06 PDT
Assuming this does not need QA before release. Please needinfo me if you think this needs testing.
Comment 23 Florian Scholz [:fscholz] (MDN) 2014-06-04 07:20:27 PDT
MAX_INTEGER got never pushed (see comment 6) and is also not part of ES6. Changing the summary.

Note You need to log in before you can comment on or make changes to this bug.