Bug 12821 – Number.toExponential doesn't work for negative numbers

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

[Mark Malone]

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.

[Mark Malone]

rdar://5007921

[Mark Malone]

Created attachment 13255 [details]
test case

[Justin Haygood]

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.

[Justin Haygood]

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

[Mark Rowe (bdash)]

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.

[Justin Haygood]

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.

[Justin Haygood]

Created attachment 13263 [details]
Same as previous with better ChangeLog.

bdash says ChangeLog needs more... added to ChangeLog.

[Mark Rowe (bdash)]

Comment on attachment 13263 [details]
Same as previous with better ChangeLog.

Looks good.  Thanks for the patch Justin.

[Sam Weinig]

Comment on attachment 13263 [details]
Same as previous with better ChangeLog.

This patch really should have a layout test with it.  Marking r-.

[Justin Haygood]

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.

[Justin Haygood]

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?

[Maciej Stachowiak]

Comment on attachment 13340 [details]
Fixes mathematical error in Number, with LayoutTest

r=me

[Alexey Proskuryakov]

See also: bug 5259.

[Alexey Proskuryakov]

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

[Justin Haygood]

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

[Alexey Proskuryakov]

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.

[Maciej Stachowiak]

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.

[Alexey Proskuryakov]

Fixed by Darin in r21256. See also: bug 5259.