Bug 44487 – Clean up NumberPrototype.cpp

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

[Gavin Barraclough]

Number.toExponential/toFixed/toPrecision all contain a spaghetti of duplicated code & unnecessary complexity.
Add a new DecimalNumber class to encapsulate double to string conversion, share the implementations of rounding & decimal-fraction/exponential formatting.

This patch changes some layout test results - in all these cases we were not previously accurate to spec defined behaviour, and we are still not - but overall this changes reduces the overall magnitude of error due to rounding differences.  The underlying problem is that we should be using dtoa to generate results to a specified accuracy, rather than relying on pre-rounding the input values.  .  We should look at reenabling our dtoa implementation to work in this fashion as a separate change.

Up to 2x progression on mircobenchmarks of toFixed/toPrecision for small results with low precision.

[Gavin Barraclough]

Created attachment 65210 [details]
The Patch

[WebKit Review Bot]

Attachment 65210 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/DecimalNumber.h:26:  #ifndef header guard has wrong style, please use: DecimalNumber_h  [build/header_guard] [5]
JavaScriptCore/wtf/DecimalNumber.h:105:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/dtoa.cpp:2295:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Attachment 65210 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3739578

[Early Warning System Bot]

Attachment 65210 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3744618

[Gavin Barraclough]

Created attachment 65213 [details]
Fix couple of issues with last patch.

[WebKit Review Bot]

Attachment 65213 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/DecimalNumber.h:105:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 65214 [details]
Fixes for JSVALUE32_64 builds.

[WebKit Review Bot]

Attachment 65214 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/DecimalNumber.h:105:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Attachment 65213 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3772598

[Early Warning System Bot]

Attachment 65213 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3733605

[Early Warning System Bot]

Attachment 65214 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3754561

[WebKit Review Bot]

Attachment 65214 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3754563

[WebKit Review Bot]

Attachment 65214 [details] did not build on win:
Build output: http://queues.webkit.org/results/3792353

[WebKit Review Bot]

Attachment 65214 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3731549

[Sam Weinig]

I love this patch, do you know what is causing all the EWS to be red?

[Gavin Barraclough]

(In reply to comment #15)
> I love this patch, do you know what is causing all the EWS to be red?

Looks like I need to include #include <math.h> for Qt, Windows will certainly need export changes.  Should be trivial things.  Will look at uploading a new patch in a bit.

[Gavin Barraclough]

Created attachment 65321 [details]
Missing include

[WebKit Review Bot]

Attachment 65321 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/wtf/DecimalNumber.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 65321 [details]
Missing include

r=me fix the bloody style issue!!!!

[Gavin Barraclough]

Fixed in r65959.