[webkit-reviews] review denied: [Bug 134980] ES6: Implement Math.sign() : [Attachment 235007] 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 134980: ES6: Implement Math.sign()
https://bugs.webkit.org/show_bug.cgi?id=134980

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235007&action=review


Looks OK, but testing is insufficient.

I’m going to say review- only because I see a coupe small mistakes in the
function and also test coverage needs to be expanded a little.

> Source/JavaScriptCore/runtime/MathObject.cpp:312
> +	   return JSValue::encode(jsNumber(PNaN));

This should be using jsNaN(), not jsNumber(PNaN), as you can see by example in
many other functions in this file.

> Source/JavaScriptCore/runtime/MathObject.cpp:314
> +	   return JSValue::encode(jsNumber(std::signbit(arg) ? -0.0 : 0));

This is likely to not get fully inlined and take a relatively slow patch to
encode the number 0, since we’re passing it in as a double and the code then
has to check that it’s a double that fits in an integer. We should instead
prefer to use this subexpression.

   std::signbit(arg) ? jsNumber(-0.0) : jsNumber(0)

This will use the integer zero. jsNumber(int) will almost certainly be
completely constant folded and compile in as a constant; it’s best to not force
the 0 to flow through jsNumber(double).

> LayoutTests/js/script-tests/math.js:261
> +shouldBe("Math.sign(NaN)", "NaN");
> +shouldBe("Math.sign(0)", "0");
> +shouldBe("Math.sign(-0)", "-0");
> +shouldBe("Math.sign(-1)", "-1");
> +shouldBe("Math.sign(1)", "1");

Not enough test cases here. This is only testing the values where sign returns
exactly what’s passed in! Should test sign(Infinity), sign(-Infinity),
sign(MAX_VALUE) and such. More like the test cases for Math.floor. Inlcude
something like sign(0.1) and sign(10000) maybe. Certainly need to cover at
least one case that’s not identity.

We also need to test behavior when passing the sign function a value that is
not a number. I know the other tests in this file don’t cover those, but that’s
largely because this file is really old. (It’s a test that comes from the khtml
project so I think it predates JavaScriptCore.)