Bug 134980 – ES6: Implement Math.sign()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Diego Pino]

Math.sign() is still pending to be implemented as part of the new ES6 Math functions.

[Diego Pino]

Created attachment 235001 [details]
Patch

[Build Bot]

Comment on attachment 235001 [details]
Patch

Attachment 235001 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5081748069154816

New failing tests:
js/math.html
js/Object-getOwnPropertyNames.html

[Build Bot]

Created attachment 235005 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Diego Pino]

Created attachment 235007 [details]
Patch

[Darin Adler]

Comment on attachment 235007 [details]
Patch

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.)

[Diego Pino]

Created attachment 235182 [details]
Patch

[Diego Pino]

Created attachment 235183 [details]
Patch

[Darin Adler]

Comment on attachment 235183 [details]
Patch

To be thorough may also later want to add test cases to show that:

1) Extra arguments are ignored.
2) Calling with no arguments yields NaN, no exception is thrown.
3) Throwing an exception in valueOf works properly, no crash.

[WebKit Commit Bot]

Comment on attachment 235183 [details]
Patch

Clearing flags on attachment: 235183

Committed r171278: <http://trac.webkit.org/changeset/171278>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.