Just me, but NUMBER_IS_NAN and NUMBER_IS_FINITE seem long. I can't understand why they prefixed them with NUMBER_. imo we should just drop that.

@bnoordhuis overall I'm diggin' the changes. this'll take some fielding on the PR's, but overall should allow for a lot better consistency across the board. plus I like the ease of search.

@bnoordhuis Here's my diff for src/macros.py. The corresponding changes will also need to be made in lib/:

diff --git a/src/macros.py b/src/macros.py
index ad54a00..3c06702 100644
--- a/src/macros.py
+++ b/src/macros.py
@@ -5,6 +5,8 @@ macro IS_NUMBER(arg)            = (typeof(arg) === 'number');
 macro IS_STRING(arg)            = (typeof(arg) === 'string');
 macro IS_SYMBOL(arg)            = (typeof(arg) === 'symbol');
 macro IS_UNDEFINED(arg)         = (typeof(arg) === 'undefined');
+macro IS_NAN(arg)               = !((arg) >= (arg));
+macro IS_FINITE(arg)            = ((arg) < 1/0 && (arg) > -1/0);

 # These macros follow the semantics of V8's %_Is*() functions.
 macro IS_ARRAY(arg)             = (Array.isArray(arg));
@@ -15,10 +17,6 @@ macro IS_REGEXP(arg)            = ((arg) instanceof RegExp);

 macro IS_BUFFER(arg)            = ((arg) instanceof Buffer);

-# TODO(bnoordhuis) Replace with optimized versions someday.
-macro NUMBER_IS_FINITE(arg)     = isFinite(arg);
-macro NUMBER_IS_NAN(arg)        = isNaN(arg);
-
 macro TO_BOOLEAN(arg)           = (!!(arg));
 macro TO_INT32(arg)             = ((arg) >> 0);
 macro TO_NUMBER(arg)            = (+(arg));

NaN is a strange bugger that can be caught with some comparison tricks. This passes all tests.

Should NUMBER_IS_NAN test that it's actually NaN, or just that it's not a number? isNaN is not going to test that the thing is NaN, only that it's NaN when coerced to Number.

To test if something actually is NaN, you can do (arg !== arg).

((arg) < 1/0 && (arg) > -1/0) is a bit more obfuscated than ((arg) < Infinity && (arg) > -Infinity)

So, what's the motivation for doing this with macros rather than just using JavaScript?

On Jul 24, 2013 5:28 PM, "Isaac Z. Schlueter" notifications@github.com
wrote:

((arg) < 1/0 && (arg) > -1/0) is a bit more obfuscated than ((arg) <
Infinity && (arg) > -Infinity)

Whatevs. That's just an old habit from a bug that used to exist in IE, but
the standard explicitly states they're the same. And hey, we're in a much
happier place now. :-)

Here's my diff for src/macros.py. The corresponding changes will also need to be made in lib/

One issue I see is that your versions evaluate their argument twice, e.g. IS_NAN(expensive(val)) becomes !(expensive(val) >= expensive(val)). Not the end of world, just something to keep in mind.

So, what's the motivation for doing this with macros rather than just using JavaScript?

Consistency, better grep factor, easier to change (you only have to update the macro.)

((arg) < 1/0 && (arg) > -1/0) is a bit more obfuscated than ((arg) < Infinity && (arg) > -Infinity)

I guess one reason for doing it like that is that it's immune to var Infinity = 42 bugs. Those shouldn't exist in core in the first place, of course.

EDIT: V8 also generates slightly better code in the 1/0 case, it saves an IC lookup.

I guess one reason for doing it like that is that it's immune to var Infinity = 42 bugs. Those shouldn't exist in core in the first place, of course.

Yes, if that's something we decide to worry about, we should also replace all our undefined usage with void 0.

EDIT: V8 also generates slightly better code in the 1/0 case, it saves an IC lookup.

Fair enough.

One issue I see is that your versions evaluate their argument twice, e.g. IS_NAN(expensive(val)) becomes !(expensive(val) >= expensive(val))

That can be avoided by wrapping in an IIFE.

macro NUMBER_IS_NAN(arg)        = ((function(x) { return x !== x; })(+arg))
macro NUMBER_IS_FINITE(arg)     = ((function(x) { return x < 1/0 && x > -1/0; })(+arg))

That can be avoided by wrapping in an IIFE.

Yeah, but that creates tiny closures all over the place. V8 won't usually (maybe never) inline them.

V8 would inline them if we defined these as JavaScript functions rather than macros.

Since isNaN and isFinite already exist, are these macros really necessary? It make sense for fancy non-obvious stuff like NUMBER_IS_UINT32, but I think the rest is probably a bit overkill.

Also! We have Object.is now! So, we could do non-casting isNaN without the double-evaluation like this:

macro NUMBER_IS_NAN(arg)        = (Object.is(arg, NaN))

But, if we cast to a number and THEN test it for NaN-ness, then it's the same as just calling isNaN

NUMBER_IS_NAN is the Number.isNaN implementation, not the global isNaN.
@isaacs that version you have will cause tests to fail. Hence why I changed
the name. Though it seems I forgot to mention that.

@bnoordhuis the check I have is very cheap in the case the developer is
consistent with passed argument types.

I generated IRHydra output for the following:

function test0(arg) { return (!(arg >= arg)) ? 0 : 1; }
function test1(arg) { return isNaN(arg) ? 0 : 1; }

function runTest() {
  var a0 = 0, a1 = 0;
  for (var i = 0; i < 1e6; i++) {
    a0 += test0(i);
    a1 += test1(i);
  }
}

runTest();

Here are he B1 blocks for test0 and test1 (where the comparison occurs) when only an smi is passed:

 B1
 v8 BlockEntry 
 v9 Simulate id=3 
v10 StackCheck t5 changes[NewSpacePromotion] 
 24 stack-check =   {} @-1 
 26 gap  ([rax|R] = [stack:-1];) 
 28 check-smi [rax|R]= [rax|R]  
 34 compare-numeric-and-branch if [rax|R] >= [rax|R] then B4 else B2

 B1
 v8 BlockEntry 
 v9 Simulate id=3 
v10 StackCheck t5 changes[NewSpacePromotion] 
t11 GlobalObject t5 
t13 Constant 0x1843c2f4fda1 <JS Function isNaN (SharedFunctionInfo 0x1843c2f2fde9)> type:object 
t15 GlobalReceiver t11 
t16 PushArgument t15 
t17 PushArgument t3 
t18 CallKnownGlobal 0x6f08d517151 <String[5]: isNaN> #2 changes[*] 
v19 Simulate id=8 push t18 
v20 Branch t18 goto (B4, B2) 
 24 stack-check =   {} @-1 
 28 global-object [rax|R]=  
 32 global-receiver [rax|R]= [rax|R] 
 36 push-argument = [rax|R] 
 38 gap  ([rax|R] = [stack:-1];) 
 40 push-argument = [rax|R] 
 44 call-known-global [rax|R]#1 /  {} @152 
 48 lazy-bailout =   
 54 branch B4 | B2 on [rax|R]

Now, the output for test1 doesn't change much for different values, but here's the output for test0 for floating point values:

 B1
 v8 BlockEntry
 v9 Simulate id=3 
v10 StackCheck t5 changes[NewSpacePromotion] 
d32 Change t3 t to d allow-undefined-as-nan 
d12 CompareNumericAndBranch GTE d32 d32 goto (B4, B2) 
 24 stack-check =   {} @-1 
 26 gap  ([rax|R] = [stack:-1];) 
 28 double-untag [xmm1|R]= [rax|R]  
 34 compare-numeric-and-branch if [xmm1|R] >= [xmm1|R] then B4 else B2

Finally if I change the input to the following:

function runTest() {
  var a0 = 0;
  var a1 = 0;
  for (var i = 0; i < 1e6; i++) {
    a0 += test0(i % 1e4 === 0 ? 'abc' : i);
    a1 += test1(i % 1e4 === 0 ? 'abc' : i);
  }
}

We get the following:

 B1
 v8 BlockEntry 
 v9 Simulate id=3 
v10 StackCheck t5 changes[NewSpacePromotion] 
t12 CompareGeneric GTE t3 t3 changes[*] type:boolean 
v13 Simulate id=8 push t12 
v14 Branch t12 goto (B4, B2) 
 24 stack-check =   {} @-1 
 26 gap  ([rdx|R] = [stack:-1]; [rax|R] = [stack:-1];) 
 28 cmp-t [rax|R]= [rdx|R] [rax|R]  @100 
 32 lazy-bailout =   
 38 branch B4 | B2 on [rax|R]

Per @isaacs request, I tested Object.is(+arg, NaN). Wan't pretty:

 B1
 v8 BlockEntry 
 v9 Simulate id=3 
v10 StackCheck t5 changes[NewSpacePromotion] 
t11 Constant 0xb91dc945ff9 <JS Function Object (SharedFunctionInfo 0xb91dc93d831)> type:object 
t44 Constant 1  range:1_1 type:smi 
t14 Mul t3 t44 ! changes[*] type:number 
v15 Simulate id=15 push t11, push t14 
t18 CheckMaps t11 [0x29c932305931] type:object 
t19 PushArgument t11 
t20 PushArgument t14 
t45 Constant nan  type:heap-number 
t21 PushArgument t45 
t22 CallConstantFunction 0x34ea09217ef1 <String[2]: is> #3 changes[*] 
v23 Simulate id=20 pop 2 / push t22 
v24 Branch t22 goto (B4, B2) 
 24 stack-check =   {} @-1 
 28 constant-t [rax|R]=  
 30 gap  ([rdx|R] = [stack:-1]; [rax|R];) 
 32 mul-t [rax|R]= [rdx|R] [rax|R]  @216 
 36 lazy-bailout =   
 40 constant-t [rbx|R]=  
 44 check-maps = [rbx|R]  
 48 push-argument = [constant:11] 
 52 push-argument = [rax|R] 
 56 push-argument = [constant:45] 
 60 call-constant-function [rax|R]#2 /  {} @213 
 64 lazy-bailout =   
 70 branch B4 | B2 on [rax|R

FWIW, I can confirm Trevor's observations: the !(arg >= arg) check generates the most efficient code (if not the smallest code, that's isNan() - but that generates an extra CALL_IC.)

We've decided to go in a separate direction for the time being. We may explore this path at a later date.