Math.sign() is still pending to be implemented as part of the new ES6 Math functions.
Created attachment 235001 [details] Patch
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
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
Created attachment 235007 [details] Patch
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.)
Created attachment 235182 [details] Patch
Created attachment 235183 [details] Patch
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.
Comment on attachment 235183 [details] Patch Clearing flags on attachment: 235183 Committed r171278: <http://trac.webkit.org/changeset/171278>
All reviewed patches have been landed. Closing bug.