Bug 16640 – Acid3 reveals broken behavior in toFixed and toExponential

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Alexey Proskuryakov]

Sounds like a duplicate of bug 5258 and bug 5259 (and yes, it's about time to fix those).

[Eric Seidel (no email)]

I have patches to fix bugs in our toFixed, toExponential and toPrecision implementations, as well as add test cases for toString.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

Created attachment 18157 [details]
Add more tests, including toPrecision

 .../fast/js/resources/toFixed-and-toExponential.js |   66 ++++++++++++++++----
 1 files changed, 54 insertions(+), 12 deletions(-)

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

I should note that these patches depend on the patches attached to bug 16643.

[Eric Seidel (no email)]

*** Bug 5258 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 5259 has been marked as a duplicate of this bug. ***

[Oliver Hunt]

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;

[Oliver Hunt]

Comment on attachment 18159 [details]
Add toString testing for firefox compatibility

whoops

Addition of failures == badness

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

The overall sunspider result is:

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

[Eric Seidel (no email)]

Landed as:
http://trac.webkit.org/projects/webkit/changeset/29020

Followup bugs filed:
http://bugs.webkit.org/show_bug.cgi?id=16651
http://bugs.webkit.org/show_bug.cgi?id=16652
http://bugs.webkit.org/show_bug.cgi?id=16653