Bug 749818 (harmony:isnan)

Harmony Number.isNaN

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: naveed, Assigned: Benjamin)

Tracking

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

unspecified
mozilla15
x86
Mac OS X
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Implement Harmony Number.isNaN http://wiki.ecmascript.org/doku.php?id=harmony:number.isnan. Not confused by type coercion
(Reporter)

Updated

5 years ago
Blocks: 694100
(Assignee)

Updated

5 years ago
Alias: harmony:isnan
(Assignee)

Comment 1

5 years ago
Created attachment 628002 [details] [diff] [review]
add Number.isNaN
Attachment #628002 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #628002 - Flags: review? → review?(jorendorff)
Comment on attachment 628002 [details] [diff] [review]
add Number.isNaN

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

::: js/src/jit-test/tests/basic/number-isnan.js
@@ +1,5 @@
> +assertEq((0/0).isNaN(), true);
> +assertEq(Number("NaN").isNaN(), true);
> +assertEq((4).isNaN(), false);
> +assertEq((4.5).isNan(), false);
> +assertEq(Number.isNaN("hi"), false);
\ No newline at end of file

You need more tests here that check for the type coercion problems with global.isNaN()
For example:
Number.isNaN("1.3") => yes
Number.isNaN("51")
Number.isNaN({valueOf: function () { return 3; })

::: js/src/jsnum.cpp
@@ +823,5 @@
>                    args);
>  }
>  
> +static JSBool
> +numObj_isNaN(JSContext *cx, unsigned argc, Value *vp)

This is not the right way to define this function. You are currently adding this as a property on Number.prototype. But the Spec (http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts&s=draft) want this to live on Number (the number constructor if you will).

Please have a look at date_static_methods in jsdate.cpp or even object_static_methods in jsobj.cpp, vm/GlobalObject.cpp

After you changed that, please mention the chapter in the spec, something we generally do for new functions. See obj_isPrototypeOf.

I think it's actually helpful to implement this function like the spec, because this way you avoid the |res|. Again obj_isPrototypeOf seems to be an good example.  Oh instead of JSBool, bool :=)
Attachment #628002 - Flags: review?(jorendorff)
> Number.isNaN("1.3") => yes
Well actually false, but whatever. (Sadly JavaScript wasn't designed so that it would be acceptable to throw for everything that isn't even a number)
Also some tests like Number.isNaN(), Number.isNaN(undefined)
(Assignee)

Updated

5 years ago
(Assignee)

Comment 4

5 years ago
Created attachment 628254 [details] [diff] [review]
address review

I'm not sure how |res| was supposed to be removed.
Assignee: general → bpeterson
Attachment #628002 - Attachment is obsolete: true
Comment on attachment 628254 [details] [diff] [review]
address review

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

Looks good. Some nit and a suggestion how to remove |res|. Really depends on what kind of style you have, but aligns with obj_isPrototypeOf and I find it easier to understand. And I have the feeling we usually prefer early returns.

::: js/src/jsnum.cpp
@@ +836,5 @@
>      JS_FS_END
>  };
>  
> +
> +// ES6 15.7.3.10

because we do that in other places:
/* ES6 draft ES6 15.7.3.10 */

@@ +841,5 @@
> +static JSBool
> +Number_isNaN(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    bool res = false;

I was thinking like that.

/* Step 1 */
if (args.length() < 1 || !args[0].isNumber) {
  args.rval.setBoolean(false);
  return true;
}

/* Step 2/3 */
args.rval.setBoolean(MOZ_DOUBLE_IS_NaN(args[0].toNumber()));
return true;
Might as well go further and check for !args[0].isDouble() there, since only a double can be NaN.
(Assignee)

Comment 7

5 years ago
Created attachment 628377 [details] [diff] [review]
now with early return
Attachment #628254 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #628377 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Attachment #628377 - Flags: review?(jorendorff) → review?(jwalden+bmo)
Comment on attachment 628377 [details] [diff] [review]
now with early return

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

::: js/src/jit-test/tests/basic/number-isnan.js
@@ +1,2 @@
> +assertEq(Number.isNaN(0/0), true);
> +assertEq(Number.isNaN(Number("NaN")), true);

assertEq(Number.isNaN(NaN), true);

@@ +3,5 @@
> +assertEq(Number.isNaN(4), false);
> +assertEq(Number.isNaN(4.5), false);
> +assertEq(Number.isNaN("hi"), false);
> +assertEq(Number.isNaN("1.3"), false);
> +assertEq(Number.isNaN("51"), false);

assertEq(Number.isNaN(0), false);
assertEq(Number.isNaN(-0), false);

@@ +5,5 @@
> +assertEq(Number.isNaN("hi"), false);
> +assertEq(Number.isNaN("1.3"), false);
> +assertEq(Number.isNaN("51"), false);
> +assertEq(Number.isNaN({valueOf: function () { return 3; }}), false);
> +assertEq(Number.isNaN({valueOf: function () { return 0/0; }}), false);

assertEq(Number.isNaN({ valueOf: function() { throw 17; } }), false);
assertEq(Number.isNaN({ toString: function() { throw 17; } }), false);
assertEq(Number.isNaN({ valueOf: function() { throw 17; }, toString: function() { throw 42; } }), false);

::: js/src/jsnum.cpp
@@ +838,5 @@
>  
> +
> +// ES6 draft ES6 15.7.3.10
> +static JSBool
> +Number_isNaN(JSContext *cx, unsigned argc, Value *vp)

Number_isNaN as a new convention for static object methods?  I like this.  We should make this change to the Object.* methods sometime for consistency.
Attachment #628377 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 628892 [details] [diff] [review]
ready for checkin
Attachment #628377 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/ba8cfc79dd6e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba8cfc79dd6e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15

Updated

5 years ago
Keywords: dev-doc-needed
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Number/isNaN
https://developer.mozilla.org/en-US/docs/Firefox_15_for_developers
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.