761495 - (harmony:inthelpers) add Number.isInteger/toInteger

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[:Benjamin Peterson]

ES 6 15.7.3.12 and 15.7.3.13

Comment 1

[:Benjamin Peterson]

Attachment: implementation

I should note I implemented this from the PDF spec not the wiki.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 630048 [details] [diff] [review]
implementation

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

Looks good.

::: js/src/jit-test/tests/basic/number-isinteger.js
@@ +3,5 @@
> +assertEq(Number.isInteger(4.2), false);
> +assertEq(Number.isInteger(0.), true);
> +assertEq(Number.isInteger(-0.), true);
> +assertEq(Number.isInteger(Infinity), true);
> +assertEq(Number.isInteger(-Infinity), true);

wat

::: js/src/jit-test/tests/basic/number-tointeger.js
@@ +2,5 @@
> +assertEq(Number.toInteger(4.), 4);
> +assertEq(Number.toInteger(4.3), 4);
> +assertEq(Number.toInteger(-4), -4);
> +assertEq(Number.toInteger(-4.), -4);
> +assertEq(Number.toInteger(-4.3), -4);

Number.toInteger(-0) should be -0, not 0, I guess? Gotta test it. :-P

(Unlike ===, assertEq() distinguishes between 0 and -0, so no problem there.)

::: js/src/jsnum.cpp
@@ +889,5 @@
> +    }
> +    double asint;
> +    if (!ToInteger(cx, args[0], &asint))
> +        return false;
> +    args.rval().setDouble(asint);

I wonder if it's worth using setNumber. It's hard to say what this will be used for, but I suspect the value will ordinarily fit in the int32 range.

It is certainly unusual to write setDouble(0.) rather than setInt32(0).

Ask bhackett which is better here, if you're curious too. Optional.

Comment 3

[:Benjamin Peterson]

Attachment: address review comments

I added a isFinite() condition to isInteger() and told Dave Herman to talk to the spec about getting that added.

You're right about using setNumber(). It's more appropriate in this case.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 630655 [details] [diff] [review]
address review comments

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac60eea3e4a

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0ac60eea3e4a

Comment 6

[Florian Scholz (Open Web Docs)]

Added to:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/16#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number#Methods

Reference pages:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isInteger
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toInteger

Note: Number.toInteger has been marked as non-standard as it has been removed on August 23, 2013 in ES6 Draft Rev 17.

Reviews welcome (as always).