here's the sample script: var val = -18450000000000000000; var res = val.toExponential(6); Expected it to return "-1.845000e+19" but got NaN instead. After further investigation it appears that this problem only exists for negative numbers.
rdar://5007921
Created attachment 13255 [details] test case
Verified, I'll take a look at it. I can't guarantee a patch, so if someone else wants to take a look, go ahead.
Created attachment 13261 [details] Inverts negative numbers so that they pass This might not be the cleanest method, but its the least intrusive, and fixes the test case, as well as the simpler one of just using -1, and pretty much every negative numbers since positive numbers do indeed pass. By using the positive codebase either way....
Comment on attachment 13261 [details] Inverts negative numbers so that they pass r- for style issues. As mentioned on IRC I'm not sure that this is the best solution to the problem. The if (!fractionDigits->isUndefined()) { ... } block is resulting in a negative x value becoming NaN. If you can work out why that happens a cleaner fix may present itself.
Created attachment 13262 [details] Fixes logic error High school math came in handy. Its a big no-no to do a log on a negative number. However, taking the absolute value of it fixes it in this case.
Created attachment 13263 [details] Same as previous with better ChangeLog. bdash says ChangeLog needs more... added to ChangeLog.
Comment on attachment 13263 [details] Same as previous with better ChangeLog. Looks good. Thanks for the patch Justin.
Comment on attachment 13263 [details] Same as previous with better ChangeLog. This patch really should have a layout test with it. Marking r-.
I can write a layout test, but due to my lack of Mac operating system, can't produce an expected results file. I tested on WebKit/Qt... I'll reattach patch with a LayoutTest once I'm at home where I can edit it.
Created attachment 13340 [details] Fixes mathematical error in Number, with LayoutTest There isn't an expected results, due to lack of a Mac and lack of working WebKit/Qt, so whoever checks this in will need to generate one for me?
Comment on attachment 13340 [details] Fixes mathematical error in Number, with LayoutTest r=me
See also: bug 5259.
I have tried to land this, but the layout tests fails (I haven't investigated why): FAIL (0).toExponential(0) should be 0e+0 (of type string). Was NaN (of type string). FAIL (0).toExponential(2) should be 0.00e+0 (of type string). Was NaN (of type string). FAIL (-1).toExponential(2) should be -1.00e+0 (of type string). Was -1e+0 (of type string). FAIL (-0.1).toExponential(2) should be -1.00e-1 (of type string). Was -1e+1 (of type string). FAIL (0.1).toExponential(2) should be 1.00e-1 (of type string). Was 1e+1 (of type string).
The layout tests was based on behavior of Mozilla Firefox. The layout tests should fail prior to landing as well. I can change the LayoutTests to match the current behavior( except for 0 failing ).
I think that a test that accompanies a bugfix should fail without the fix, and pass (with correct results) with it. Just changing shouldBe() expectations to match the current results (rather than expected ones) would be rather confusing. The included test shows that toExponential() is still pretty much broken even in very basic cases. If I were working on in and not seeing a simple fix, I'd probably comment out the failing cases in the test, and file separate bug(s) for them.
Comment on attachment 13340 [details] Fixes mathematical error in Number, with LayoutTest Moving to r- based on Alexey's comments - we don't want to check in a new test in a way that fails right out of the gate. That does not do a very good job of showing the fix. Better would be something that fails before and passes after.
Fixed by Darin in r21256. See also: bug 5259.