Last Comment Bug 761495 - (harmony:inthelpers) add Number.isInteger/toInteger
(harmony:inthelpers)
: add Number.isInteger/toInteger
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla16
Assigned To: :Benjamin Peterson
:
:
Mentors:
Depends on:
Blocks: es6
  Show dependency treegraph
 
Reported: 2012-06-04 20:56 PDT by :Benjamin Peterson
Modified: 2013-10-11 12:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implementation (3.74 KB, patch)
2012-06-04 20:58 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review
address review comments (3.92 KB, patch)
2012-06-06 11:58 PDT, :Benjamin Peterson
jorendorff: review+
Details | Diff | Splinter Review

Description :Benjamin Peterson 2012-06-04 20:56:10 PDT
ES 6 15.7.3.12 and 15.7.3.13
Comment 1 :Benjamin Peterson 2012-06-04 20:58:03 PDT
Created attachment 630048 [details] [diff] [review]
implementation

I should note I implemented this from the PDF spec not the wiki.
Comment 2 Jason Orendorff [:jorendorff] 2012-06-05 19:56:00 PDT
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 2012-06-06 11:58:37 PDT
Created attachment 630655 [details] [diff] [review]
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] 2012-06-06 19:55:28 PDT
Comment on attachment 630655 [details] [diff] [review]
address review comments

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac60eea3e4a
Comment 5 Ed Morley [:emorley] 2012-06-07 05:46:23 PDT
https://hg.mozilla.org/mozilla-central/rev/0ac60eea3e4a

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