The default bug view has changed. See this FAQ.
Bug 761495 (harmony:inthelpers)

add Number.isInteger/toInteger

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Benjamin, Assigned: Benjamin)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla16
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
ES 6 15.7.3.12 and 15.7.3.13
(Assignee)

Comment 1

5 years ago
Created attachment 630048 [details] [diff] [review]
implementation

I should note I implemented this from the PDF spec not the wiki.
Assignee: general → bpeterson
Attachment #630048 - Flags: review?(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.
Attachment #630048 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 3

5 years ago
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.
Attachment #630048 - Attachment is obsolete: true
Comment on attachment 630655 [details] [diff] [review]
address review comments

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac60eea3e4a
Attachment #630655 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0ac60eea3e4a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16

Updated

5 years ago
Keywords: dev-doc-needed
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).
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.