Closed Bug 1003764 Opened 10 years ago Closed 10 years ago

Implement ES6 Number.isSafeInteger()

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: till, Assigned: nathan)

References

(Blocks 1 open bug, )

Details

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

Attachments

(1 file, 2 obsolete files)

Steve, would you by any chance be interested in working on this? It's sort of a follow-up to the {MAX,MIN}_SAFE_INTEGER things you fixed.

In case you are: look at the number_static_methods array in jsnum.cpp and the there-defined static functions on Number for pointers on how to implement this.
Flags: needinfo?(steveire)
Whiteboard: [DocArea=JS]
Assigning to Nathan as a basic easing-in bug for JS work, seeing as Steve hasn't responded to the needinfo yet.  :-)
Assignee: general → nathan
Flags: needinfo?(steveire)
Assignee: nathan → miloignis
First draft of patch, added self-hosted implementation and testcase.
Attachment #8430233 - Flags: review?(jwalden+bmo)
Comment on attachment 8430233 [details] [diff] [review]
Patch implementing isSafeInteger and testcase

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

Looks good, with the minor changes noted.  You're missing tests, tho -- gotta have those to land.  (Maybe your ~/.hgrc is missing

[diff]
git = 1
showfunc = true

?)  With adequate tests this is ready to land.

(Minor process notes!

Every patch that lands, except for super-trivial ones like uninteresting comment fixes, spelling fixes, &c. has to be reviewed before it can land.  r+ with no other comments on a patch means it's good to land as-is [or as-is with an updated commit message that includes the bug number and a mention of the review -- as sometimes patches are posted at the same time the bug's filed, or when review is uncertain, or whatever].  r+ with comments means those comments should be addressed before the patch can land, but the reviewer doesn't think the changes are so complex to require another pass.  r- means the patch needs work and shouldn't land.  It's a bit of a judgment call distinguishing r+ with comments from r-.

Oh, there's also canceling the r? entirely -- more or less equivalent to r-, just possibly a more gentle version of it.  :-)

And then there's feedback, where f+ is sort of "I like this approach but am not explicitly giving it a review" or "I like this approach, and you should polish it into a reviewable state".)

::: js/src/builtin/Number.js
@@ +41,5 @@
> +
> +// ES6 draft ES6 20.1.2.5
> +function Number_isSafeInteger(number) {
> +
> +    //Step 1.

Remove the blank line at the top of the function, and make the comments "// Step N." for a little extra breathing space when reading.  See for example the "// Step 5." in the function just above.

@@ +42,5 @@
> +// ES6 draft ES6 20.1.2.5
> +function Number_isSafeInteger(number) {
> +
> +    //Step 1.
> +    if (typeof number != 'number')

!== instead of !=, because loose equality is just broken.  :-)  Self-hosted code should almost never use ==/!==, with extremely rare exceptions.

@@ +46,5 @@
> +    if (typeof number != 'number')
> +        return false;
> +
> +    //Step 2.
> +    if (std_isNaN(number) || !std_isFinite(number))

Interestingly, it seems neither of these methods is inlined.  Probably because they're not used in benchmarks, maybe because they're both new in ES6.  Let's fix that.  Separate bug, of course -- and a new area of code to start working in.  :-)

Actually, tho.  "If number is NaN, +Inf, or -Inf" is equivalent to "If number is not finite" the way we use the word "finite", which is equivalent to the behavior of !Number.isFinite.  So you only need the second of these checks, not the first.

@@ +56,5 @@
> +    //Step 4.
> +    if (integer != number)
> +        return false;
> +
> +    //Step 5. If abs(integer) =< 2^53 - 1, return true.

Mini-nitpick, but I prefer ** to ^ because the latter is ambiguous with xor.

::: js/src/builtin/Utilities.js
@@ +49,5 @@
>  var std_Function_apply = Function.prototype.apply;
>  var std_Math_floor = Math.floor;
>  var std_Math_max = Math.max;
>  var std_Math_min = Math.min;
> +var std_Math_abs = Math.abs;

For future reference, Math.abs (and a bunch of other functions) will be inlined by JITs when possible.  See jit/MCallOptimize.cpp for the full list.  So for such functions, no need to worry about overhead of performing that operation using a function call instead of with inlined code.
Attachment #8430233 - Flags: review?(jwalden+bmo) → feedback+
Attachment #8430233 - Attachment is obsolete: true
Yikes! Sorry, I didn't realize the test didn't get added to the patch.
I think I fixed the mentioned items.
Thanks!
Attachment #8430394 - Flags: review?(jwalden+bmo)
Comment on attachment 8430394 [details] [diff] [review]
Fixed issues from review, added in the test that got left out last time

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

One little last pass on this, looking fine in general, tho.

::: js/src/builtin/Number.js
@@ +52,5 @@
> +    // Step 3.
> +    var integer = ToInteger(number);
> +
> +    // Step 4.
> +    if (integer != number)

!== here as well.

@@ +55,5 @@
> +    // Step 4.
> +    if (integer != number)
> +        return false;
> +
> +    // Step 5. If abs(integer) =< 2**53 - 1, return true.

<= in the comment, or even ≤ if you want to get all fancy.

::: js/src/tests/ecma_6/Number/isSafeInteger-01.js
@@ +5,5 @@
> +var BUGNUMBER = 1003764;
> +var summary = "ES6 (draft Draft May 22, 2014) ES6 20.1.2.5 Number.isSafeInteger(number)";
> +
> +print(BUGNUMBER + ": " + summary);
> +

It's a minor testing nitpick, but the value of the "length" property of the function is also part of the interface, so it too should be tested.

assertEq(Number.isSafeInteger.length, 1);

The 1 derives from the algorithm being defined with "(number)" in the spec, and that implying length 1 through the prose in Chapter 17 of the spec.

@@ +13,5 @@
> +assertEq(Number.isSafeInteger(+Infinity), false);
> +assertEq(Number.isSafeInteger(-Infinity), false);
> +
> +assertEq(Number.isSafeInteger(-1), true);
> +assertEq(Number.isSafeInteger(0), true);

As JS numbers are IEEE-754, any test for zero should be expanded into tests for both -0 and +0.  JS equality operations equate -0 and +0, and mailing list discussion indicates that -0 is to be considered safe.  (Probably we should file a spec bug to be more explicit about -0 handling, just as an aside.)

@@ +18,5 @@
> +assertEq(Number.isSafeInteger(1), true);
> +
> +assertEq(Number.isSafeInteger(3.2), false);
> +assertEq(Number.isSafeInteger(9007199254740992), false);
> +assertEq(Number.isSafeInteger(-9007199254740992), false);

Generally you should test a value before, a value at, and a value after the boundary.  Also, to double-check the literal number in the implementation, I'd do this with Math.pow.  So, then:

assertEq(Number.isSafeInteger(Math.pow(2, 53) - 2), true);
assertEq(Number.isSafeInteger(Math.pow(2, 53) - 1), true);
assertEq(Number.isSafeInteger(Math.pow(2, 53)), false);
assertEq(Number.isSafeInteger(Math.pow(2, 53) + 1), false);
assertEq(Number.isSafeInteger(Math.pow(2, 53) + 2), false);

assertEq(Number.isSafeInteger(-Math.pow(2, 53) - 2), false);
assertEq(Number.isSafeInteger(-Math.pow(2, 53) - 1), false);
assertEq(Number.isSafeInteger(-Math.pow(2, 53)), false);
assertEq(Number.isSafeInteger(-Math.pow(2, 53) + 1), true);
assertEq(Number.isSafeInteger(-Math.pow(2, 53) + 2), true);

(Technically 2**53 + 1 and -2**53 - 1 evaluate to 2**53 and -2**53, in IEEE-754 and ES6 semantics.  But doing it this way doesn't make people wonder why there might be a jump from 2**53 to 2**53 + 2, say.)
Attachment #8430394 - Flags: review?(jwalden+bmo)
Awesome, I should have covered the points you mentioned.
Thanks for the review!
Attachment #8430394 - Attachment is obsolete: true
Attachment #8430434 - Flags: review?(jwalden+bmo)
Comment on attachment 8430434 [details] [diff] [review]
Tidied up and added in the new test cases

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

Woo!  Will push this for you shortly.
Attachment #8430434 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc4e6faf697

Interestingly this triggered apparent CLOBBER bustage.  I wonder if our setup for generating the self-hosted code in the build system is dependency-broken somehow...  :-\
You need to log in before you can comment on or make changes to this bug.