Bug 44487

Summary: Clean up NumberPrototype.cpp
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: JavaScriptCoreAssignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, gustavo, sam, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
The Patch
none
Fix couple of issues with last patch.
none
Fixes for JSVALUE32_64 builds.
none
Missing include oliver: review+

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