Last Comment Bug 749818 - (harmony:isnan) Harmony Number.isNaN
(harmony:isnan)
: Harmony Number.isNaN
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: :Benjamin Peterson
:
: Jason Orendorff [:jorendorff]
Mentors:
http://wiki.ecmascript.org/doku.php?i...
Depends on:
Blocks: es6
  Show dependency treegraph
 
Reported: 2012-04-27 15:49 PDT by naveed
Modified: 2012-12-04 13:19 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add Number.isNaN (2.02 KB, patch)
2012-05-29 10:02 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
address review (2.58 KB, patch)
2012-05-30 00:19 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review
now with early return (2.58 KB, patch)
2012-05-30 10:21 PDT, :Benjamin Peterson
jwalden+bmo: review+
Details | Diff | Splinter Review
ready for checkin (2.94 KB, patch)
2012-05-31 14:06 PDT, :Benjamin Peterson
no flags Details | Diff | Splinter Review

Description naveed 2012-04-27 15:49:22 PDT
Implement Harmony Number.isNaN http://wiki.ecmascript.org/doku.php?id=harmony:number.isnan. Not confused by type coercion
Comment 1 :Benjamin Peterson 2012-05-29 10:02:22 PDT
Created attachment 628002 [details] [diff] [review]
add Number.isNaN
Comment 2 Tom Schuster [:evilpie] 2012-05-29 11:22:12 PDT
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 Schuster [:evilpie] 2012-05-29 13:07:32 PDT
> 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 2012-05-30 00:19:50 PDT
Created attachment 628254 [details] [diff] [review]
address review

I'm not sure how |res| was supposed to be removed.
Comment 5 Tom Schuster [:evilpie] 2012-05-30 01:34:53 PDT
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] (remove +bmo to email) 2012-05-30 02:26:05 PDT
Might as well go further and check for !args[0].isDouble() there, since only a double can be NaN.
Comment 7 :Benjamin Peterson 2012-05-30 10:21:23 PDT
Created attachment 628377 [details] [diff] [review]
now with early return
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-31 11:53:44 PDT
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 2012-05-31 14:06:14 PDT
Created attachment 628892 [details] [diff] [review]
ready for checkin
Comment 11 Phil Ringnalda (:philor) 2012-06-03 12:17:05 PDT
https://hg.mozilla.org/mozilla-central/rev/ba8cfc79dd6e

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