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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 238436: Patch
https://bugs.webkit.org/attachment.cgi?id=238436&action=review

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