Bug 16640 - Acid3 reveals broken behavior in toFixed and toExponential
Summary: Acid3 reveals broken behavior in toFixed and toExponential
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: EasyFix
: 5258 5259 (view as bug list)
Depends on: 16643
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-28 02:15 PST by Eric Seidel (no email)
Modified: 2007-12-28 20:19 PST (History)
1 user (show)

See Also:


Attachments
Test case for toFixed/toExponential (1.18 KB, patch)
2007-12-28 02:39 PST, Eric Seidel (no email)
oliver: review+
Details | Formatted Diff | Diff
Test case for toFixed/toExponential (1.18 KB, patch)
2007-12-28 15:13 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add more toFixed and toExponential tests, fix (-0).toFixed() and refactor a little (4.81 KB, patch)
2007-12-28 15:13 PST, Eric Seidel (no email)
oliver: review+
Details | Formatted Diff | Diff
Fix -0.toExponential() and printing of trailing 0s in toExponential (5.11 KB, patch)
2007-12-28 15:13 PST, Eric Seidel (no email)
oliver: review+
Details | Formatted Diff | Diff
Add more tests, including toPrecision (5.52 KB, patch)
2007-12-28 15:13 PST, Eric Seidel (no email)
oliver: review+
Details | Formatted Diff | Diff
Split tests into 3 files, fixed toPrecision(nan) handling (18.31 KB, patch)
2007-12-28 15:13 PST, Eric Seidel (no email)
oliver: review+
Details | Formatted Diff | Diff
Add toString testing for firefox compatibility (24.15 KB, patch)
2007-12-28 15:13 PST, Eric Seidel (no email)
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-12-28 02:15:09 PST
Acid3 reveals broken behavior in toFixed and toExponential

This section of the test fails miserably:

      if ((0.0).toFixed(4) != "0.0000" ||
         (-0.0).toFixed(4) != "0.0000")
        ok = false;
      if ((0.0).toExponential(4) != "0.0000e+0" ||
         (-0.0).toExponential(4) != "0.0000e+0")
        ok = false;

I can't imagine this is intentional.  Current behavior:

(0.0).toFixed(4)
0.0000
(-0.0).toFixed(4)
0.00-0
(0.0).toExponential(4)
NaN
(-0.0).toExponential(4)
NaN
Comment 1 Eric Seidel (no email) 2007-12-28 02:39:24 PST
Created attachment 18143 [details]
Test case for toFixed/toExponential

 .../fast/js/resources/toFixed-and-toExponential.js |    6 ++++++
 LayoutTests/fast/js/toFixed-and-toExponential.html |   13 +++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)
Comment 2 Eric Seidel (no email) 2007-12-28 02:39:51 PST
I don't have the energy tonight to actually make the fix, but the attached test case patch, makes it very clear what is broken.
Comment 3 Alexey Proskuryakov 2007-12-28 06:58:53 PST
Sounds like a duplicate of bug 5258 and bug 5259 (and yes, it's about time to fix those).
Comment 4 Eric Seidel (no email) 2007-12-28 15:13:08 PST
I have patches to fix bugs in our toFixed, toExponential and toPrecision implementations, as well as add test cases for toString.
Comment 5 Eric Seidel (no email) 2007-12-28 15:13:35 PST
Created attachment 18154 [details]
Test case for toFixed/toExponential

 .../fast/js/resources/toFixed-and-toExponential.js |    6 ++++++
 LayoutTests/fast/js/toFixed-and-toExponential.html |   13 +++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)
Comment 6 Eric Seidel (no email) 2007-12-28 15:13:37 PST
Created attachment 18155 [details]
Add more toFixed and toExponential tests, fix (-0).toFixed() and refactor a little

 JavaScriptCore/kjs/number_object.cpp               |   37 +++++++++++--------
 .../fast/js/resources/toFixed-and-toExponential.js |   38 ++++++++++++++++++++
 2 files changed, 59 insertions(+), 16 deletions(-)
Comment 7 Eric Seidel (no email) 2007-12-28 15:13:39 PST
Created attachment 18156 [details]
Fix -0.toExponential() and printing of trailing 0s in toExponential

 JavaScriptCore/kjs/number_object.cpp               |   51 +++++++++++--------
 .../fast/js/resources/toFixed-and-toExponential.js |    5 ++
 2 files changed, 34 insertions(+), 22 deletions(-)
Comment 8 Eric Seidel (no email) 2007-12-28 15:13:41 PST
Created attachment 18157 [details]
Add more tests, including toPrecision

 .../fast/js/resources/toFixed-and-toExponential.js |   66 ++++++++++++++++----
 1 files changed, 54 insertions(+), 12 deletions(-)
Comment 9 Eric Seidel (no email) 2007-12-28 15:13:44 PST
Created attachment 18158 [details]
Split tests into 3 files, fixed toPrecision(nan) handling

 JavaScriptCore/kjs/number_object.cpp               |    2 +-
 LayoutTests/fast/js/number-tofixed-expected.txt    |   24 +++++
 .../fast/js/number-toprecision-expected.txt        |   26 +++++-
 .../fast/js/resources/number-toExponential.js      |   43 +++++++++
 LayoutTests/fast/js/resources/number-tofixed.js    |   44 ++++++++++
 .../fast/js/resources/number-toprecision.js        |   38 ++++++++-
 .../fast/js/resources/toFixed-and-toExponential.js |   91 --------------------
 LayoutTests/fast/js/toFixed-and-toExponential.html |   13 ---
 8 files changed, 170 insertions(+), 111 deletions(-)
Comment 10 Eric Seidel (no email) 2007-12-28 15:13:46 PST
Created attachment 18159 [details]
Add toString testing for firefox compatibility

 .../fast/js/number-toExponential-expected.txt      |   27 ++++++++
 LayoutTests/fast/js/number-toString-expected.txt   |   58 +++++++++++++++++
 LayoutTests/fast/js/number-toString.html           |   13 ++++
 LayoutTests/fast/js/resources/number-toString.js   |   67 ++++++++++++++++++++
 4 files changed, 165 insertions(+), 0 deletions(-)
Comment 11 Eric Seidel (no email) 2007-12-28 15:14:23 PST
I should note that these patches depend on the patches attached to bug 16643.
Comment 12 Eric Seidel (no email) 2007-12-28 15:16:27 PST
*** Bug 5258 has been marked as a duplicate of this bug. ***
Comment 13 Eric Seidel (no email) 2007-12-28 15:16:58 PST
*** Bug 5259 has been marked as a duplicate of this bug. ***
Comment 14 Oliver Hunt 2007-12-28 18:41:07 PST
Comment on attachment 18156 [details]
Fix -0.toExponential() and printing of trailing 0s in toExponential

I don't believe you can safely check against -0.0

+    if (x == -0.0) // (-0.0).toExponential() should print as 0 instead of -0
+        x = 0;
Comment 15 Oliver Hunt 2007-12-28 18:44:35 PST
Comment on attachment 18159 [details]
Add toString testing for firefox compatibility

whoops

Addition of failures == badness
Comment 16 Eric Seidel (no email) 2007-12-28 19:57:45 PST
Sunspider reports somewhat wild results (some faster some slower) as a result of these changes.  None seem related, and I write it off as random gcc code-gen changes. :(
Comment 17 Eric Seidel (no email) 2007-12-28 19:59:42 PST
The overall sunspider result is:

** TOTAL **:           ??                3881.8ms +/- 0.2%   3886.3ms +/- 0.2%     not conclusive: might be *1.001x as slow*