[Webkit-unassigned] [Bug 131707] Simple ES6 feature: Number constructor extras

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #238086|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #11 from Darin Adler <darin at apple.com>  2014-09-14 13:34:36 PST ---
(From update of attachment 238086)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.