[webkit-reviews] review denied: [Bug 131707] Simple ES6 feature: Number constructor extras : [Attachment 238086] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Sep 14 13:34:33 PDT 2014


Darin Adler <darin at apple.com> has denied Diego Pino <dpino at igalia.com>'s
request for review:
Bug 131707: Simple ES6 feature: Number constructor extras
https://bugs.webkit.org/show_bug.cgi?id=131707

Attachment 238086: Patch
https://bugs.webkit.org/attachment.cgi?id=238086&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list