[webkit-reviews] review granted: [Bug 131707] Simple ES6 feature: Number constructor extras : [Attachment 238436] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Sep 21 17:08:24 PDT 2014
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.
More information about the webkit-reviews
mailing list