[Webkit-unassigned] [Bug 134980] ES6: Implement Math.sign()

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #5 from Darin Adler <darin at apple.com>  2014-07-19 22:13:55 PST ---
(From update of attachment 235007)
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.)

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