Bug 134980 - ES6: Implement Math.sign()
Summary: ES6: Implement Math.sign()
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Minor
Assignee: Nobody
Depends on:
Reported: 2014-07-16 09:38 PDT by Diego Pino
Modified: 2014-07-20 09:17 PDT (History)
3 users (show)

See Also:

Patch (5.96 KB, patch)
2014-07-16 09:44 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (577.59 KB, application/zip)
2014-07-16 10:19 PDT, Build Bot
no flags Details
Patch (8.24 KB, patch)
2014-07-16 11:14 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (8.84 KB, patch)
2014-07-20 06:17 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (9.18 KB, patch)
2014-07-20 06:22 PDT, Diego Pino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Pino 2014-07-16 09:38:28 PDT
Math.sign() is still pending to be implemented as part of the new ES6 Math functions.
Comment 1 Diego Pino 2014-07-16 09:44:34 PDT
Created attachment 235001 [details]
Comment 2 Build Bot 2014-07-16 10:19:19 PDT
Comment on attachment 235001 [details]

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

New failing tests:
Comment 3 Build Bot 2014-07-16 10:19:21 PDT
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
Comment 4 Diego Pino 2014-07-16 11:14:56 PDT
Created attachment 235007 [details]
Comment 5 Darin Adler 2014-07-19 22:13:55 PDT
Comment on attachment 235007 [details]

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.)
Comment 6 Diego Pino 2014-07-20 06:17:45 PDT
Created attachment 235182 [details]
Comment 7 Diego Pino 2014-07-20 06:22:41 PDT
Created attachment 235183 [details]
Comment 8 Darin Adler 2014-07-20 08:46:01 PDT
Comment on attachment 235183 [details]

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.
Comment 9 WebKit Commit Bot 2014-07-20 09:17:23 PDT
Comment on attachment 235183 [details]

Clearing flags on attachment: 235183

Committed r171278: <http://trac.webkit.org/changeset/171278>
Comment 10 WebKit Commit Bot 2014-07-20 09:17:26 PDT
All reviewed patches have been landed.  Closing bug.