Closed Bug 761495 (harmony:inthelpers) Opened 12 years ago Closed 12 years ago

add Number.isInteger/toInteger

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: Benjamin, Assigned: Benjamin)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

ES 6 15.7.3.12 and 15.7.3.13
Attached patch implementation (obsolete) — Splinter Review
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+
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
https://hg.mozilla.org/mozilla-central/rev/0ac60eea3e4a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.