Bug 44487 - Clean up NumberPrototype.cpp
Summary: Clean up NumberPrototype.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-23 22:25 PDT by Gavin Barraclough
Modified: 2010-08-24 18:14 PDT (History)
7 users (show)

See Also:


Attachments
The Patch (69.52 KB, patch)
2010-08-23 22:43 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fix couple of issues with last patch. (72.25 KB, patch)
2010-08-23 23:23 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fixes for JSVALUE32_64 builds. (72.04 KB, patch)
2010-08-23 23:27 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Missing include (72.06 KB, patch)
2010-08-24 14:09 PDT, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2010-08-23 22:25:02 PDT
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.
Comment 1 Gavin Barraclough 2010-08-23 22:43:06 PDT
Created attachment 65210 [details]
The Patch
Comment 2 WebKit Review Bot 2010-08-23 22:47:29 PDT
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.
Comment 3 Eric Seidel (no email) 2010-08-23 22:55:13 PDT
Attachment 65210 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3739578
Comment 4 Early Warning System Bot 2010-08-23 22:59:59 PDT
Attachment 65210 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3744618
Comment 5 Gavin Barraclough 2010-08-23 23:23:06 PDT
Created attachment 65213 [details]
Fix couple of issues with last patch.
Comment 6 WebKit Review Bot 2010-08-23 23:25:01 PDT
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.
Comment 7 Gavin Barraclough 2010-08-23 23:27:56 PDT
Created attachment 65214 [details]
Fixes for JSVALUE32_64 builds.
Comment 8 WebKit Review Bot 2010-08-23 23:31:10 PDT
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.
Comment 9 Eric Seidel (no email) 2010-08-23 23:36:22 PDT
Attachment 65213 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3772598
Comment 10 Early Warning System Bot 2010-08-23 23:40:07 PDT
Attachment 65213 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3733605
Comment 11 Early Warning System Bot 2010-08-23 23:55:20 PDT
Attachment 65214 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3754561
Comment 12 WebKit Review Bot 2010-08-24 00:36:48 PDT
Attachment 65214 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3754563
Comment 13 WebKit Review Bot 2010-08-24 01:29:26 PDT
Attachment 65214 [details] did not build on win:
Build output: http://queues.webkit.org/results/3792353
Comment 14 WebKit Review Bot 2010-08-24 01:35:34 PDT
Attachment 65214 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3731549
Comment 15 Sam Weinig 2010-08-24 08:21:43 PDT
I love this patch, do you know what is causing all the EWS to be red?
Comment 16 Gavin Barraclough 2010-08-24 12:36:10 PDT
(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.
Comment 17 Gavin Barraclough 2010-08-24 14:09:10 PDT
Created attachment 65321 [details]
Missing include
Comment 18 WebKit Review Bot 2010-08-24 14:12:02 PDT
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.
Comment 19 Oliver Hunt 2010-08-24 15:41:27 PDT
Comment on attachment 65321 [details]
Missing include

r=me fix the bloody style issue!!!!
Comment 20 Gavin Barraclough 2010-08-24 18:14:50 PDT
Fixed in r65959.