Bug 12821 - Number.toExponential doesn't work for negative numbers
Summary: Number.toExponential doesn't work for negative numbers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Justin Haygood
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2007-02-19 16:33 PST by Mark Malone
Modified: 2007-05-04 10:43 PDT (History)
2 users (show)

See Also:


Attachments
test case (587 bytes, text/html)
2007-02-19 16:35 PST, Mark Malone
no flags Details
Inverts negative numbers so that they pass (1.45 KB, patch)
2007-02-19 19:22 PST, Justin Haygood
mrowe: review-
Details | Formatted Diff | Diff
Fixes logic error (995 bytes, patch)
2007-02-19 19:45 PST, Justin Haygood
no flags Details | Formatted Diff | Diff
Same as previous with better ChangeLog. (1.36 KB, patch)
2007-02-19 19:57 PST, Justin Haygood
sam: review-
Details | Formatted Diff | Diff
Fixes mathematical error in Number, with LayoutTest (3.83 KB, patch)
2007-02-22 21:46 PST, Justin Haygood
mjs: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Malone 2007-02-19 16:33:30 PST
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.
Comment 1 Mark Malone 2007-02-19 16:34:01 PST
rdar://5007921
Comment 2 Mark Malone 2007-02-19 16:35:51 PST
Created attachment 13255 [details]
test case
Comment 3 Justin Haygood 2007-02-19 18:50:12 PST
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.
Comment 4 Justin Haygood 2007-02-19 19:22:00 PST
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 5 Mark Rowe (bdash) 2007-02-19 19:39:05 PST
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.
Comment 6 Justin Haygood 2007-02-19 19:45:53 PST
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.
Comment 7 Justin Haygood 2007-02-19 19:57:43 PST
Created attachment 13263 [details]
Same as previous with better ChangeLog.

bdash says ChangeLog needs more... added to ChangeLog.
Comment 8 Mark Rowe (bdash) 2007-02-19 19:59:11 PST
Comment on attachment 13263 [details]
Same as previous with better ChangeLog.

Looks good.  Thanks for the patch Justin.
Comment 9 Sam Weinig 2007-02-20 08:04:50 PST
Comment on attachment 13263 [details]
Same as previous with better ChangeLog.

This patch really should have a layout test with it.  Marking r-.
Comment 10 Justin Haygood 2007-02-20 14:46:08 PST
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.
Comment 11 Justin Haygood 2007-02-22 21:46:34 PST
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 12 Maciej Stachowiak 2007-02-22 23:25:53 PST
Comment on attachment 13340 [details]
Fixes mathematical error in Number, with LayoutTest

r=me
Comment 13 Alexey Proskuryakov 2007-02-23 10:57:27 PST
See also: bug 5259.
Comment 14 Alexey Proskuryakov 2007-02-27 10:52:52 PST
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).
Comment 15 Justin Haygood 2007-02-27 11:38:35 PST
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 ).
Comment 16 Alexey Proskuryakov 2007-02-27 12:17:44 PST
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 17 Maciej Stachowiak 2007-02-27 23:00:26 PST
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.
Comment 18 Alexey Proskuryakov 2007-05-04 10:43:53 PDT
Fixed by Darin in r21256. See also: bug 5259.