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

[bugzilla-daemon at webkit.org]

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 238259: Patch
https://bugs.webkit.org/attachment.cgi?id=238259&action=review

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