749818 - (harmony:isnan) Harmony Number.isNaN

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[naveed]

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

Comment 1

[:Benjamin Peterson]

Attachment: add Number.isNaN

Comment 2

[Tom S]

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 :=)

Comment 3

[Tom S]

> 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)

Comment 4

[:Benjamin Peterson]

Attachment: address review

I'm not sure how |res| was supposed to be removed.

Comment 5

[Tom S]

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;

Comment 6

[Jeff Walden [:Waldo]]

Might as well go further and check for !args[0].isDouble() there, since only a double can be NaN.

Comment 7

[:Benjamin Peterson]

Attachment: now with early return

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 9

[:Benjamin Peterson]

Attachment: ready for checkin

Comment 10

[Tom S]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ba8cfc79dd6e

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ba8cfc79dd6e

Comment 12

[Tom S]

https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Number/isNaN
https://developer.mozilla.org/en-US/docs/Firefox_15_for_developers