Bug 131707 – Simple ES6 feature: Number constructor extras

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

Number.isFinite
Number.isInteger
Number.isNaN
Number.toInteger

The two integer functions are referring to _Doubles_ not int32 so they're essentially hasFractionalComponent, truncateFranctionalComponent

isFinite is the C version where
-0 => -0
 0  => 0

[Radar WebKit Bug Importer]

<rdar://problem/16626161>

[Oliver Hunt]

Last part of description was intended for the next bug

[Diego Pino]

Created attachment 238027 [details]
Patch

[Diego Pino]

Implemented new ES6 constants and functions in Number constructor (Section 20.1.2 onwards http://people.mozilla.org/~jorendorff/es6-draft.html#sec-number.epsilon). 

Number.toInteger is no longer part of the spec.

[Diego Pino]

Created attachment 238083 [details]
Patch

[Build Bot]

Comment on attachment 238083 [details]
Patch

Attachment 238083 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5855082465722368

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 238084 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 238083 [details]
Patch

Attachment 238083 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5968588787679232

New failing tests:
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 238085 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Diego Pino]

Created attachment 238086 [details]
Patch

[Darin Adler]

Comment on attachment 238086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238086&action=review

Great start, needs some work.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:96
> +    if (isFunction(propertyName.publicName())) {

Can use uid() instead of publicName() for better efficiency.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:108
> +bool ALWAYS_INLINE NumberConstructor::isFunction(String propertyName)
> +{
> +    return propertyName == "isFinite" || propertyName == "isInteger" || propertyName == "isNaN"
> +        || propertyName == "isSafeInteger" || propertyName == "parseFloat" || propertyName == "parseInt";
> +}

This is the wrong way to do something like this. The reason we have the class Identifier is so we can do pointer comparisons rather than string comparisons for something like this. Also, writing the function like this causes reference count churn as the string gets copied into the argument. To see how we normally do this, look for uses of CommonIdentifiers and the propertyNames data member of VM.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:113
> +    return JSValue::encode(jsNumber(std::pow(2, -52)));

I’m not sure the compile will constant fold std::pow here. Can we use std::numeric_limits<double>::epsilon() or DBL_EPSILON instead, or do those have the wrong value?

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:149
> +    return JSValue::encode(jsNumber(std::pow(2, 53) - 1));

Same question as above. Is there some way to express this that gets constant-folded?

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:155
> +    return JSValue::encode(jsNumber(-(std::pow(2, 53) - 1)));

Same question as above. Is there some way to express this that gets constant-folded?

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:194
> +    JSValue arg0 = exec->argument(0);
> +    if (!arg0.isNumber())
> +        return JSValue::encode(jsBoolean(false));
> +    double number = arg0.toNumber(exec);
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());
> +    return JSValue::encode(jsBoolean(std::isfinite(number)));

Unnecessarily complex and inefficient. It should be more like this:

    JSValue argument = exec->argument(0);
    bool isFinite = argument.isNumber() && (!argument.isDouble() || std::isfinite(argument.asDouble());
    return JSValue::encode(jsBoolean(isFinite));

Could also do it without the isFinite local variable.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:206
> +    JSValue arg0 = exec->argument(0);
> +    if (!arg0.isNumber())
> +        return JSValue::encode(jsBoolean(false));
> +    double integer = arg0.toInteger(exec);
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());
> +    return JSValue::encode(jsBoolean(std::isfinite(integer) && integer == arg0.toNumber(exec)));

Something like this:

    JSValue argument = exec->argument(0);
    bool isInteger;
    if (argument.isInt32())
         isInteger = true;
    else if (!argument.isDouble())
         isInteger = false;
    else {
        double number = argument.asDouble();
         isInteger = number == static_cast<int64_t>(double);
    }
    return JSValue::encode(jsBoolean(isInteger));

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:218
> +    JSValue arg0 = exec->argument(0);
> +    if (!arg0.isNumber())
> +        return JSValue::encode(jsBoolean(false));
> +    double number = arg0.toNumber(exec);
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());
> +    return JSValue::encode(jsBoolean(std::isnan(number)));

Something like this:

    JSValue argument = exec->argument(0);
    return JSValue::encode(jsBoolean(argument.isDouble() && std::isnan(argument.asDouble()));

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:232
> +    JSValue arg0 = exec->argument(0);
> +    if (!arg0.isNumber())
> +        return JSValue::encode(jsBoolean(false));
> +    double number = arg0.toInteger(exec);
> +    if (exec->hadException())
> +        return JSValue::encode(jsUndefined());
> +    if (number != arg0.toNumber(exec))
> +        return JSValue::encode(jsBoolean(false));
> +    return JSValue::encode(jsBoolean(std::abs(number) <= (std::pow(2, 53) - 1)));

Something like this:

    return JSValue::encode(jsBoolean(exec->argument(0).isMachineInt()));

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:245
> +// ECMA-262 20.1.2.12
> +static EncodedJSValue JSC_HOST_CALL numberConstructorFuncParseFloat(ExecState* exec)
> +{
> +    return globalFuncParseFloat(exec); 
> +}
> +
> +// ECMA-262 20.1.2.13
> +static EncodedJSValue JSC_HOST_CALL numberConstructorFuncParseInt(ExecState* exec)
> +{
> +    return globalFuncParseInt(exec); 
> +}

We should use the existing functions directly to avoid an extra level of function call indirection.

> Source/JavaScriptCore/runtime/NumberConstructor.h:61
> +    static bool isFunction(String propertyName);

Not the right way to do this. Function should take AtomicStringImpl*, not String, and probably needs a VM& so it can look up the property names as common identifiers.

> LayoutTests/js/script-tests/number-constructor.js:1
> +description('This test case tests the Number constructor.');

You’ll note that there are no test cases here that cover an exception. That’s because valueOf never gets called in any of these cases. That’s the clue that the exception handling code we added in this patch isn’t actually needed. The fact that it can’t be tested is a clue!

> LayoutTests/js/script-tests/number-constructor.js:3
> +// isFinite

Should also test the values around MAX_SAFE_INTEGER and MIN_SAFE_INTEGER. Should also test minimums and maximums.

> LayoutTests/js/script-tests/number-constructor.js:22
> +// isInteger

Should also test the values around MAX_SAFE_INTEGER and MIN_SAFE_INTEGER. Should also test minimums and maximums.

> LayoutTests/js/script-tests/number-constructor.js:41
> +// isNaN

Should also test the values around MAX_SAFE_INTEGER and MIN_SAFE_INTEGER. Should also test minimums and maximums.

> LayoutTests/js/script-tests/number-constructor.js:60
> +// isSafeInteger

Should also test minimums and maximums.

> LayoutTests/js/script-tests/number-constructor.js:87
> +// parseFloat

Where are the edge cases here? The really long numbers, like things that parse to minimums and maximums or just beyond.

> LayoutTests/js/script-tests/number-constructor.js:107
> +// parseInt

Where are the edge cases here? The really long numbers, like things that parse to minimums and maximums or just beyond.

[Diego Pino]

Comment on attachment 238086 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238086&action=review

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:194
>> +    return JSValue::encode(jsBoolean(std::isfinite(number)));
> 
> Unnecessarily complex and inefficient. It should be more like this:
> 
>     JSValue argument = exec->argument(0);
>     bool isFinite = argument.isNumber() && (!argument.isDouble() || std::isfinite(argument.asDouble());
>     return JSValue::encode(jsBoolean(isFinite));
> 
> Could also do it without the isFinite local variable.

isDouble() is the same as isNumber() && !isInt32(). Since argument is already a number, !argument.isDouble() could be written as argument.isInt32(). My understanding is that this check is here as an optimization, to avoid executing std::isfinite().

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:206
>> +    return JSValue::encode(jsBoolean(std::isfinite(integer) && integer == arg0.toNumber(exec)));
> 
> Something like this:
> 
>     JSValue argument = exec->argument(0);
>     bool isInteger;
>     if (argument.isInt32())
>          isInteger = true;
>     else if (!argument.isDouble())
>          isInteger = false;
>     else {
>         double number = argument.asDouble();
>          isInteger = number == static_cast<int64_t>(double);
>     }
>     return JSValue::encode(jsBoolean(isInteger));

It doesn't work for Number.MAX_VALUE, which is 1.79E+308 (or 2^1024). Casting Number.MAX_VALUE to int64_t gets 9223372036854775807 (2^64 - 1).

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:218
>> +    return JSValue::encode(jsBoolean(std::isnan(number)));
> 
> Something like this:
> 
>     JSValue argument = exec->argument(0);
>     return JSValue::encode(jsBoolean(argument.isDouble() && std::isnan(argument.asDouble()));

OK.

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:232
>> +    return JSValue::encode(jsBoolean(std::abs(number) <= (std::pow(2, 53) - 1)));
> 
> Something like this:
> 
>     return JSValue::encode(jsBoolean(exec->argument(0).isMachineInt()));

It seems it doesn't work for MAX_SAFE_INTEGER, MAX_SAFE_INTEGER - 1, MIN_SAFE_INTEGER and MIN_SAFE_INTEGER + 1. Strange.

[Diego Pino]

Created attachment 238259 [details]
Patch

[Darin Adler]

Comment on attachment 238259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238259&action=review

Much better. Getting really close. Still not quite right.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:157
> +    return JSValue::encode(jsNumber(-9007199254740991LL));

I believe this is the wrong value. I believe that -9007199254740992 is safe.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:204
> +        double integer = argument.toInteger(exec);

We really don't want to call all the way through toInteger, which can handle a result of any type, just to get the:

    std::isnan(d) ? 0.0 : trunc(d)

Code that is inside the toInteger function. We should rewrite this to not call toInteger. As long as we have sufficient test coverage that should be fine.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:205
> +        isInteger = std::isfinite(integer) && argument.asDouble() == integer;

I don’t think the isfinite check is needed in the current version of this. If we rewrite to not use toInteger above we may or may not need this.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:228
> +        double integer = argument.toInteger(exec);
> +        isInteger = argument.asDouble() == integer && std::abs(integer) <= 9007199254740991LL;

I believe this isn’t correct; it's going to incorrectly return false for the maximum negative integer.

But also, my comments above about not using toInteger apply here too.

[Diego Pino]

Comment on attachment 238259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238259&action=review

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:157
>> +    return JSValue::encode(jsNumber(-9007199254740991LL));
> 
> I believe this is the wrong value. I believe that -9007199254740992 is safe.

The ES6 spec says the value of MIN_SAFE_VALUE is -9007199254740991 http://people.mozilla.org/~jorendorff/es6-draft.html#sec-number.min_safe_integer

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:205
>> +        isInteger = std::isfinite(integer) && argument.asDouble() == integer;
> 
> I don’t think the isfinite check is needed in the current version of this. If we rewrite to not use toInteger above we may or may not need this.

The std:isfinite(integer) is needed to discard values Number.POSITIVE_INFINITY and Number.NEGATIVE_INFINITY. The previous checks do not discard these values.

[Diego Pino]

Created attachment 238436 [details]
Patch

[Darin Adler]

Comment on attachment 238436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238436&action=review

OK to land like this, but I think I spotted one more problem.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:115
> +    return JSValue::encode(jsNumber(std::numeric_limits<double>::epsilon()));

Should use jsDoubleNumber instead of jsNumber for better efficiency.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:127
>      return JSValue::encode(jsNumber(-std::numeric_limits<double>::infinity()));

Existing code, but should use jsDoubleNumber instead of jsNumber for better efficiency.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:133
>      return JSValue::encode(jsNumber(std::numeric_limits<double>::infinity()));

Existing code, but should use jsDoubleNumber instead of jsNumber for better efficiency.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:139
>      return JSValue::encode(jsNumber(1.7976931348623157E+308));

Existing code, but should use jsDoubleNumber instead of jsNumber for better efficiency.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:145
>      return JSValue::encode(jsNumber(5E-324));

Existing code, but should use jsDoubleNumber instead of jsNumber for better efficiency.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:151
> +    return JSValue::encode(jsNumber(9007199254740991LL));

Should be jsDoubleNumber(9007199254740991.0) for better efficiency; avoids converting from long long to double and also some other checking.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:157
> +    return JSValue::encode(jsNumber(-9007199254740991LL));

Should be jsDoubleNumber(-9007199254740991.0) for better efficiency; avoids converting from long long to double and also some other checking.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:205
> +        isInteger = std::isfinite(number) && trunc(number) == number;

I think this might return true for -0 and I believe it should return false.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:228
> +        isInteger = trunc(number) == number && std::abs(number) <= 9007199254740991LL;

Should be 9007199254740991.0, not 9007199254740991LL.

I think this might return true for -0 and I believe it should return false.

> LayoutTests/js/script-tests/number-constructor.js:4
> +shouldBeTrue('Number.isFinite(0)');

Need to test negative zero.

> LayoutTests/js/script-tests/number-constructor.js:27
> +shouldBeTrue('Number.isInteger(0)');

Need to test negative zero.

> LayoutTests/js/script-tests/number-constructor.js:51
> +shouldBeFalse('Number.isNaN(0)');

Need to test negative zero.

> LayoutTests/js/script-tests/number-constructor.js:73
> +shouldBeTrue('Number.isSafeInteger(0)');

Need to test negative zero.

> LayoutTests/js/script-tests/number-constructor.js:100
> +shouldBe('Number.parseFloat("0")', '0');

Need to test negative zero.

> LayoutTests/js/script-tests/number-constructor.js:126
> +shouldBe('Number.parseInt("0")', '0');

Need to test negative zero.

[Diego Pino]

Comment on attachment 238436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238436&action=review

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:228
>> +        isInteger = trunc(number) == number && std::abs(number) <= 9007199254740991LL;
> 
> Should be 9007199254740991.0, not 9007199254740991LL.
> 
> I think this might return true for -0 and I believe it should return false.

isSafeInteger(-0) returns true, but I think it makes sense. That's the reason why the range of safe integers is [-9007199254740991, 9007199254740991] and not [-9007199254740992, 9007199254740991]. Zero is a signed number in the range of safe integers, so Number.isSafeInteger(-0) must return true. The same applies for is isFinite() and isInteger(). To confirm, I also tested these cases in V8 and SpiderMonkey and both return isFinite(-0), isInteger(-0) and isSafeInteger(-0) true.

[Diego Pino]

Created attachment 238471 [details]
Patch

[Darin Adler]

Comment on attachment 238471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238471&action=review

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:115
> +    return JSValue::encode(jsDoubleNumber((std::numeric_limits<double>::epsilon())));

Should come back here and remove the extra parentheses.

[WebKit Commit Bot]

Comment on attachment 238471 [details]
Patch

Clearing flags on attachment: 238471

Committed r173839: <http://trac.webkit.org/changeset/173839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Filip Pizlo]

Comment on attachment 238471 [details]
Patch

I really don't like this approach.  Why are all of these constants implemented as getters?  Is that what the spec mandates?

[Filip Pizlo]

I've thought about this more.  I think that the performance cliffs that you've introduced by the way that you've implemented the new NumberPrototype properties are so severe that we shouldn't allow this code to ever be exposed to users.

I recommend rolling out this patch, and changing NumberPrototype to no longer use static tables.  See MathObject for how this is done.  Then the constant properties can be implemented as just constants.

[Diego Pino]

(In reply to comment #23)
> (From update of attachment 238471 [details])
> I really don't like this approach.  Why are all of these constants implemented as getters?  Is that what the spec mandates?

No, not really. There were already some constants implemented, like Number.MAX_VALUE, and I followed the same approach. I was not aware of the performance penalties.

I'm OK with rolling out the patch. I can recode it using MathObject approach.

[Diego Pino]

(In reply to comment #24)

> I recommend rolling out this patch, and changing NumberPrototype to no longer use static tables.  See MathObject for how this is done.  Then the constant properties can be implemented as just constants.

I have patch ready following MathObject style. Can anyone revert the patch so I can upload the new one?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 137062

[Diego Pino]

Created attachment 238593 [details]
Patch

[Darin Adler]

Seems a shame we reverted the patch. Would have been fine to land the improved patch as a refinement to the original rather than starting over.

[Darin Adler]

Comment on attachment 238593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238593&action=review

> LayoutTests/js/script-tests/number-constructor.js:22
> +shouldBeFalse('Number.isFinite(undefined)');

Just for your information, there is no keyword “undefined”. This just works because undefined is not the name of any variable or property. Could also be “foo”, which is also undefined.

> LayoutTests/js/script-tests/number-constructor.js:64
> +shouldBeFalse('Number.isNaN(false)');

true?

> LayoutTests/js/script-tests/number-constructor.js:91
> +shouldBeFalse('Number.isSafeInteger(false)');

true?

> LayoutTests/js/script-tests/number-constructor.js:119
> +shouldBe('Number.parseFloat(false)', 'NaN');

true?

> LayoutTests/js/script-tests/number-constructor.js:121
> +shouldBe('Number.parseFloat(null)', 'NaN');

no test of undefined for parseFloat?

> LayoutTests/js/script-tests/number-constructor.js:149
> +shouldBe('Number.parseInt(false)', 'NaN');

true?

> LayoutTests/js/script-tests/number-constructor.js:151
> +shouldBe('Number.parseInt(null)', 'NaN');

no test of undefined for parseInt?

[Diego Pino]

Created attachment 238769 [details]
Patch

[Diego Pino]

(In reply to comment #30)
> (From update of attachment 238593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238593&action=review
> 
> > LayoutTests/js/script-tests/number-constructor.js:22
> > +shouldBeFalse('Number.isFinite(undefined)');
> 
> Just for your information, there is no keyword “undefined”. This just works because undefined is not the name of any variable or property. Could also be “foo”, which is also undefined.

I looked a bit into this. It seems "undefined" is a property of the global object, so it's a variable in the global scope. The value of "undefined" is the primitive value undefined. You're correct when you say "undefined" is not a reserved-word in JavaScript. It would be possible to do "var undefined = 10;", although this property of the global object is non-writable, non-configurable, so it has no effect.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/undefined

Number.isFinite(foo) wouldn't work because "foo" is not defined as a variable yet (throws Exception ReferenceError). Number.isFinite(undefined) is the same as Number.isFinite(), in both cases the argument the function isFinite(x) receives will be undefined.

For all the functions (except in the cases of parseInt and parseFloat), there was already a test for testing a function without arguments and calling it with "undefined" . Since it is the same thing, I will remove the "undefined" calls. I will add a test for calling a function with a not defined variable, should throw an Exception.

> 
> > LayoutTests/js/script-tests/number-constructor.js:64
> > +shouldBeFalse('Number.isNaN(false)');
> 
> true?
> 

Correct.

> > LayoutTests/js/script-tests/number-constructor.js:121
> > +shouldBe('Number.parseFloat(null)', 'NaN');
> 
> no test of undefined for parseFloat?
>

Same thing as Number.parseFloat().

[Oliver Hunt]

(In reply to comment #30)
> (From update of attachment 238593 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238593&action=review
> 
> > LayoutTests/js/script-tests/number-constructor.js:22
> > +shouldBeFalse('Number.isFinite(undefined)');
> 
> Just for your information, there is no keyword “undefined”. This just works because undefined is not the name of any variable or property. Could also be “foo”, which is also undefined.

Happily ES spec does now define an immutable property named "undefined" with the value undefined on the global object!

Yay!

Alas it's still not a keyword so 
function f(undefined) { if (typeof blah === undefined) ... }

Is still horribly broken, but if you do that you deserve whatever pain you get :)

[Darin Adler]

Comment on attachment 238593 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238593&action=review

>>>> LayoutTests/js/script-tests/number-constructor.js:22
>>>> +shouldBeFalse('Number.isFinite(undefined)');
>>> 
>>> Just for your information, there is no keyword “undefined”. This just works because undefined is not the name of any variable or property. Could also be “foo”, which is also undefined.
>> 
>> I looked a bit into this. It seems "undefined" is a property of the global object, so it's a variable in the global scope. The value of "undefined" is the primitive value undefined. You're correct when you say "undefined" is not a reserved-word in JavaScript. It would be possible to do "var undefined = 10;", although this property of the global object is non-writable, non-configurable, so it has no effect.
>> 
>> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/undefined
>> 
>> Number.isFinite(foo) wouldn't work because "foo" is not defined as a variable yet (throws Exception ReferenceError). Number.isFinite(undefined) is the same as Number.isFinite(), in both cases the argument the function isFinite(x) receives will be undefined.
>> 
>> For all the functions (except in the cases of parseInt and parseFloat), there was already a test for testing a function without arguments and calling it with "undefined" . Since it is the same thing, I will remove the "undefined" calls. I will add a test for calling a function with a not defined variable, should throw an Exception.
> 
> Happily ES spec does now define an immutable property named "undefined" with the value undefined on the global object!
> 
> Yay!
> 
> Alas it's still not a keyword so 
> function f(undefined) { if (typeof blah === undefined) ... }
> 
> Is still horribly broken, but if you do that you deserve whatever pain you get :)

Got it. That window object property named "undefined" is new since I learned JavaScript.

>>> LayoutTests/js/script-tests/number-constructor.js:121
>>> +shouldBe('Number.parseFloat(null)', 'NaN');
>> 
>> no test of undefined for parseFloat?
> 
> Same thing as Number.parseFloat().

I don’t think you understand my question. Inside the engine, it’s possible for Number.parseFloat() and Number.parseFloat(variableWithUndefinedValue) to work differently. Functions defined in the JavaScript language treat them the same way, but it’s possible to make an error and treat them differently, so both should be tested.

For example, there are functions in the HTML DOM that treat them differently (much to my chagrin).

[WebKit Commit Bot]

Comment on attachment 238769 [details]
Patch

Clearing flags on attachment: 238769

Committed r174049: <http://trac.webkit.org/changeset/174049>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 238769 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238769&action=review

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:125
> +    macro(isFinite) \
> +    macro(isInteger) \
> +    macro(isNaN) \
>      macro(isPrototypeOf) \
> +    macro(isSafeInteger) \

Wait, why are we adding these? I don’t think this change should have been included in the patch.

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:148
> +    macro(parseFloat) \
> +    macro(parseInt) \

Ditto.

[Diego Pino]

(In reply to comment #37)
> (From update of attachment 238769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238769&action=review
> 
> > Source/JavaScriptCore/runtime/CommonIdentifiers.h:125
> > +    macro(isFinite) \
> > +    macro(isInteger) \
> > +    macro(isNaN) \
> >      macro(isPrototypeOf) \
> > +    macro(isSafeInteger) \
> 
> Wait, why are we adding these? I don’t think this change should have been included in the patch.
> 
> > Source/JavaScriptCore/runtime/CommonIdentifiers.h:148
> > +    macro(parseFloat) \
> > +    macro(parseInt) \
> 
> Ditto.

Sorry, my fault. I created a new bug fixing this issue and adding tests for Number.parseFloat(undefined) and Number.parseInt(undefined).

https://bugs.webkit.org/show_bug.cgi?id=137206