Issue 15333006: Rewrite double.parse.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+277 lines, -186 lines)			Patch
M	runtime/lib/double.cc	View	1 2 3 4	1 chunk	+14 lines, -80 lines	0 comments	Download
M	runtime/lib/double_patch.dart	View	1 2 3 4	1 chunk	+35 lines, -5 lines	0 comments	Download
M	runtime/vm/dart_api_impl_test.cc	View	1 2 3 4 5 6	1 chunk	+58 lines, -0 lines	0 comments	Download
M	sdk/lib/_internal/compiler/implementation/lib/js_helper.dart	View	1 2 3 4 5 6	1 chunk	+9 lines, -5 lines	0 comments	Download
M	sdk/lib/core/double.dart	View	1 2 3 4 5 6	1 chunk	+23 lines, -7 lines	0 comments	Download
M	tests/co19/co19-dart2js.status	View	1 2 3 4 5 6 7 8	2 chunks	+2 lines, -1 line	0 comments	Download
M	tests/co19/co19-runtime.status	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
M	tests/compiler/dart2js/js_parser_test.dart	View	1 2 3 4 5 6 7 8	1 chunk	+0 lines, -1 line	0 comments	Download
M	tests/corelib/string_trim2_test.dart	View	1 2 3 4 5 6 7	2 chunks	+6 lines, -5 lines	0 comments	Download
M	tests/lib/math/math_parse_double_test.dart	View	1 2 3 4 5 6 7 8	2 chunks	+129 lines, -81 lines	0 comments	Download

Messages

Total messages: 18 (0 generated)

Expand Messages | Collapse Messages

srdjan

https://codereview.chromium.org/15333006/diff/1/runtime/lib/double.cc File runtime/lib/double.cc (right): https://codereview.chromium.org/15333006/diff/1/runtime/lib/double.cc#newcode206 runtime/lib/double.cc:206: if (!value.IsOneByteString() || len == 0) { Please add ...

7 years, 7 months ago (2013-05-17 23:19:06 UTC) #3

srdjan

https://codereview.chromium.org/15333006/diff/3001/runtime/lib/integers_patch.dart File runtime/lib/integers_patch.dart (right): https://codereview.chromium.org/15333006/diff/3001/runtime/lib/integers_patch.dart#newcode16 runtime/lib/integers_patch.dart:16: if (!str._isWhitespace(str.codeUnitAt(ix))) break; Synchronize with Siva's upcoming change that ...

7 years, 7 months ago (2013-05-17 23:40:28 UTC) #4

Ivan Posva

DBC -ip https://codereview.chromium.org/15333006/diff/3001/runtime/lib/double.cc File runtime/lib/double.cc (right): https://codereview.chromium.org/15333006/diff/3001/runtime/lib/double.cc#newcode206 runtime/lib/double.cc:206: if (!value.IsOneByteString() || len == 0) { ...

7 years, 7 months ago (2013-05-18 00:45:43 UTC) #5

floitsch

7 years, 7 months ago (2013-05-20 13:15:29 UTC) #6

Ivan Posva

-ip https://codereview.chromium.org/15333006/diff/3001/runtime/lib/double.cc File runtime/lib/double.cc (right): https://codereview.chromium.org/15333006/diff/3001/runtime/lib/double.cc#newcode206 runtime/lib/double.cc:206: if (!value.IsOneByteString() || len == 0) { On ...

7 years, 7 months ago (2013-05-20 16:00:23 UTC) #7

srdjan

7 years, 7 months ago (2013-05-20 17:08:58 UTC) #8

Lasse Reichstein Nielsen

Not convinced that format changes are a good idea. https://codereview.chromium.org/15333006/diff/17001/runtime/lib/integers_patch.dart File runtime/lib/integers_patch.dart (right): https://codereview.chromium.org/15333006/diff/17001/runtime/lib/integers_patch.dart#newcode16 runtime/lib/integers_patch.dart:16: ...

7 years, 7 months ago (2013-05-21 06:12:09 UTC) #9

Lasse Reichstein Nielsen

https://codereview.chromium.org/15333006/diff/1/runtime/lib/double_patch.dart File runtime/lib/double_patch.dart (right): https://codereview.chromium.org/15333006/diff/1/runtime/lib/double_patch.dart#newcode49 runtime/lib/double_patch.dart:49: return _handleParseError(str, handleError); _handleParseError is only used once, so ...

7 years, 7 months ago (2013-05-21 06:15:06 UTC) #10

floitsch

https://codereview.chromium.org/15333006/diff/3001/runtime/lib/double.cc File runtime/lib/double.cc (right): https://codereview.chromium.org/15333006/diff/3001/runtime/lib/double.cc#newcode206 runtime/lib/double.cc:206: if (!value.IsOneByteString() || len == 0) { On 2013/05/20 ...

7 years, 7 months ago (2013-05-21 14:39:31 UTC) #11

https://codereview.chromium.org/15333006/diff/3001/runtime/lib/double.cc
File runtime/lib/double.cc (right):

https://codereview.chromium.org/15333006/diff/3001/runtime/lib/double.cc#newc...
runtime/lib/double.cc:206: if (!value.IsOneByteString() || len == 0) {
On 2013/05/20 16:00:23, Ivan Posva wrote:
> On 2013/05/20 13:15:29, floitsch wrote:
> > On 2013/05/18 00:45:43, Ivan Posva wrote:
> > > What about an external one byte string? Or an external two byte string
that
> > only
> > > contains Latin1 values, which is a legal situation to be in.
> > 
> > They would have been converted to OneByteStrings in the Dart part.
> 
> A comment to that effect would be great.
> 
> On a separate note: I do think it would be beneficial to allow external one
byte
> strings as well, no?

Done.

https://codereview.chromium.org/15333006/diff/3001/runtime/lib/integers_patch...
File runtime/lib/integers_patch.dart (right):

https://codereview.chromium.org/15333006/diff/3001/runtime/lib/integers_patch...
runtime/lib/integers_patch.dart:16: if (!str._isWhitespace(str.codeUnitAt(ix)))
break;
On 2013/05/20 17:08:59, srdjan wrote:
> On 2013/05/20 13:15:29, floitsch wrote:
> > On 2013/05/17 23:40:28, srdjan wrote:
> > > Synchronize with Siva's upcoming change that makes it static.
> > 
> > Ok. I changed it back to method call, so that it wouldn't check for
whitespace
> > with value > 256 (for onebyte strings).
> > How can a static do that?
> 
> I think we should expect str to be OneByteString, abort or converts if is not.

Ok. In any case I reverted the changes.

https://codereview.chromium.org/15333006/diff/17001/runtime/lib/double_patch....
File runtime/lib/double_patch.dart (right):

https://codereview.chromium.org/15333006/diff/17001/runtime/lib/double_patch....
runtime/lib/double_patch.dart:19: if ((ccid != _OneByteString._classId)) {
On 2013/05/20 16:00:23, Ivan Posva wrote:
> I think it would be OK to have a _ExternalOneByteString here as well.

I tried to do this, but _ExternalOneByteString doesn't have any _classId getter
yet. Added a TODO instead.

https://codereview.chromium.org/15333006/diff/17001/runtime/lib/integers_patc...
File runtime/lib/integers_patch.dart (right):

https://codereview.chromium.org/15333006/diff/17001/runtime/lib/integers_patc...
runtime/lib/integers_patch.dart:16: if (!str._isWhitespace(str.codeUnitAt(ix)))
break;
On 2013/05/21 06:12:09, Lasse Reichstein Nielsen wrote:
> This is a hack that deserves explanation. 
> Anybody would expect _isWhitespace to be static/top-level here.
> Is this really faster than using the same function all the time, and starting
it
> with a (unit <= 127) branch?
> 
> Assign str to a dynamic variable before doing this call. It should generate a
> warning, since _isWhiteSpace is not on the String class.
> 

Has been removed.

https://codereview.chromium.org/15333006/diff/17001/sdk/lib/core/double.dart
File sdk/lib/core/double.dart (right):

https://codereview.chromium.org/15333006/diff/17001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:137: *
`/^\s*[+-]?(?:Infinity|NaN|(?:\.\d+|\d+(?:\.\d*)?)(?:[eE][+-]?\d+)?)\s*$`.
On 2013/05/21 06:12:09, Lasse Reichstein Nielsen wrote:
> Don't use RegExp as specification. It's unreadable to the common user (and
> (P-space) hard to read for those who do can read regexps).
> 
> I don't want to accept leading or trailing whitespace. The user can remove it
> using String.trim if necessary - no need to also implement trim in this
> function. It seems the previous implementations accepted it though, so we
might
> need to keep it for consistency.
> 
> Also accepts "4." which is not a valid literal (and wasn't allowed before).
> Please disallow this.
> 
> Is +4 a valid literal in the spec any more? (Both implementations accept it,
but
> I seem to remember it being removed).

Removed the regexp.
As discussed keeping the new behavior.

https://codereview.chromium.org/15333006/diff/17001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:140: * "NaN" or a double number. A double number is
composed of the mantissa and
On 2013/05/21 06:12:09, Lasse Reichstein Nielsen wrote:
> This accepts -NaN, which is meaningless. Please avoid that as well.

As discussed. keeping.

Lasse Reichstein Nielsen

lgtm https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart File sdk/lib/core/double.dart (right): https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#newcode136 sdk/lib/core/double.dart:136: * Accepts an optional sign followed by either ...

7 years, 7 months ago (2013-05-22 08:47:26 UTC) #13

lgtm

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart
File sdk/lib/core/double.dart (right):

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:136: * Accepts an optional sign followed by either
"Infinity",
.. optional sign (`+` or `-`) immediately followed by either the characters
"Infinity", the characters "NaN" or a ...

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:137: * "NaN" or a floating-point representation. A
floating-point number is
"floating-point representation", then "floating point number". Probably change
the "number" to "representation".

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:138: * composed of the mantissa and and optional
exponent part. The mantissa is
"the mantissa" -> "a mantissa"
"and and" -> "and an".

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:139: * either a dot followed by a sequence of digits,
or a sequence of digits
"dot" -> "decimal point (`.`)"
"digts" -> "(decimal) digits"

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:140: * optionally followed by a dot and more digits.
The (optional) exponent part
This disallows "4." (which I approve of!) but probably isn't what you want.
"dot" -> "decimal point".

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:141: * consists of the character "e" or "E", an
optional sign, and the exponent
"and the exponent digit(s)" -> "and one or more digits"

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:144: * The whole string may be surrounded by
whitespace.
Refer to [String.trim] for a definition of "whitepsace", if possible.

https://codereview.chromium.org/15333006/diff/30001/tests/corelib/string_trim...
File tests/corelib/string_trim2_test.dart (right):

https://codereview.chromium.org/15333006/diff/30001/tests/corelib/string_trim...
tests/corelib/string_trim2_test.dart:8: 9,
Consider using hex for the low numbers too.

https://codereview.chromium.org/15333006/diff/30001/tests/corelib/string_trim...
tests/corelib/string_trim2_test.dart:47: }
Consider running through all other characters and see that they are not trimmed.
There's only ~2^21 of them :)
At least take the first 2^16.

https://codereview.chromium.org/15333006/diff/30001/tests/lib/math/math_parse...
File tests/lib/math/math_parse_double_test.dart (right):

https://codereview.chromium.org/15333006/diff/30001/tests/lib/math/math_parse...
tests/lib/math/math_parse_double_test.dart:132: Expect.equals(double.INFINITY,
double.parse(" +Infinity"));
Make a parameterized test that takes a string and test, and then does the test
with and without whitespace. E.g.:

  testParse(string, bool test(result)) {
    for (var s in [string, " $string", "$string ", "\xa0 $string", ...) {
      test(double.parse(s));
    }
  }

https://codereview.chromium.org/15333006/diff/30001/tests/lib/math/math_parse...
tests/lib/math/math_parse_double_test.dart:162: 
More edge case tests (positive and negative):
5e-324  // min-pos
0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004940656458412465441765687928682213723650598026143247644255856825006755072702087518652998363616359923797965646954457177309266567103559397963987747960107818781263007131903114045278458171678489821036887186360569987307230500063874091535649843873124733972731696151400317153853980741262385655911710266585566867681870395603106249319452715914924553293054565444011274801297099995419319894090804165633245247571478690147267801593552386115501348035264934720193790268107107491703332226844753335720832431936092382893458368060106011506169809753078342277318329247904982524730776375927247874656084778203734469699533647017972677717585125660551199131504891101451037862738167250955837389733598993664809941164205702637090279242767544565229087538682506419718265533447265625
 // Same, without exponential.
2e-324  // underflow 0.0
0.9999999999999999  // max below 1
1.00000000000000005 // 1.0
1.0000000000000002  // min above 1
2147483647  // max int32
2147483647.0000002  // min not int32
2147483648  // min int not int32
4295967295  // max uint32
4295967295.000001  // min not uint-32
4295967296  // min int not-uint32
1.7976931348623157e+308  // Max finit
1.7976931348623158e+308  // Infinity
.049999999999999994    // not 0.5
.04999999999999999935  // 0.5
4503599627370497.5  // 4503499627370498
(more exponentials as non-exponential)

floitsch

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart File sdk/lib/core/double.dart (right): https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#newcode136 sdk/lib/core/double.dart:136: * Accepts an optional sign followed by either "Infinity", ...

7 years, 7 months ago (2013-05-23 02:30:03 UTC) #14

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart
File sdk/lib/core/double.dart (right):

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:136: * Accepts an optional sign followed by either
"Infinity",
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> .. optional sign (`+` or `-`) immediately followed by either the characters
> "Infinity", the characters "NaN" or a ...

Done.

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:137: * "NaN" or a floating-point representation. A
floating-point number is
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> "floating-point representation", then "floating point number". Probably change
> the "number" to "representation".

Done.

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:138: * composed of the mantissa and and optional
exponent part. The mantissa is
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> "the mantissa" -> "a mantissa"
> "and and" -> "and an".

Done.

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:139: * either a dot followed by a sequence of digits,
or a sequence of digits
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> "dot" -> "decimal point (`.`)"
> "digts" -> "(decimal) digits"

Done.

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:140: * optionally followed by a dot and more digits.
The (optional) exponent part
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> This disallows "4." (which I approve of!) but probably isn't what you want.
> "dot" -> "decimal point".

Changed to "and optionally more digits. Did not repeat the "decimal".

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:141: * consists of the character "e" or "E", an
optional sign, and the exponent
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> "and the exponent digit(s)" -> "and one or more digits"

Done.

https://codereview.chromium.org/15333006/diff/30001/sdk/lib/core/double.dart#...
sdk/lib/core/double.dart:144: * The whole string may be surrounded by
whitespace.
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> Refer to [String.trim] for a definition of "whitepsace", if possible.

Changed to: "The input string is trimmed (see [String.trim]) before conversion.

https://codereview.chromium.org/15333006/diff/30001/tests/corelib/string_trim...
File tests/corelib/string_trim2_test.dart (right):

https://codereview.chromium.org/15333006/diff/30001/tests/corelib/string_trim...
tests/corelib/string_trim2_test.dart:8: 9,
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> Consider using hex for the low numbers too.

Done.

https://codereview.chromium.org/15333006/diff/30001/tests/corelib/string_trim...
tests/corelib/string_trim2_test.dart:47: }
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> Consider running through all other characters and see that they are not
trimmed.
> There's only ~2^21 of them :)
> At least take the first 2^16.

I really think that would be too slow. Convince me in person if you think
otherwise.

https://codereview.chromium.org/15333006/diff/30001/tests/lib/math/math_parse...
File tests/lib/math/math_parse_double_test.dart (right):

https://codereview.chromium.org/15333006/diff/30001/tests/lib/math/math_parse...
tests/lib/math/math_parse_double_test.dart:132: Expect.equals(double.INFINITY,
double.parse(" +Infinity"));
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> Make a parameterized test that takes a string and test, and then does the test
> with and without whitespace. E.g.:
> 
>   testParse(string, bool test(result)) {
>     for (var s in [string, " $string", "$string ", "\xa0 $string", ...) {
>       test(double.parse(s));
>     }
>   }

Done.

https://codereview.chromium.org/15333006/diff/30001/tests/lib/math/math_parse...
tests/lib/math/math_parse_double_test.dart:162: 
On 2013/05/22 08:47:26, Lasse Reichstein Nielsen wrote:
> More edge case tests (positive and negative):
> 5e-324  // min-pos
>
0.000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000004940656458412465441765687928682213723650598026143247644255856825006755072702087518652998363616359923797965646954457177309266567103559397963987747960107818781263007131903114045278458171678489821036887186360569987307230500063874091535649843873124733972731696151400317153853980741262385655911710266585566867681870395603106249319452715914924553293054565444011274801297099995419319894090804165633245247571478690147267801593552386115501348035264934720193790268107107491703332226844753335720832431936092382893458368060106011506169809753078342277318329247904982524730776375927247874656084778203734469699533647017972677717585125660551199131504891101451037862738167250955837389733598993664809941164205702637090279242767544565229087538682506419718265533447265625
>  // Same, without exponential.
> 2e-324  // underflow 0.0
> 0.9999999999999999  // max below 1
> 1.00000000000000005 // 1.0
> 1.0000000000000002  // min above 1
> 2147483647  // max int32
> 2147483647.0000002  // min not int32
> 2147483648  // min int not int32
> 4295967295  // max uint32
> 4295967295.000001  // min not uint-32
> 4295967296  // min int not-uint32
> 1.7976931348623157e+308  // Max finit
> 1.7976931348623158e+308  // Infinity
> .049999999999999994    // not 0.5
> .04999999999999999935  // 0.5
> 4503599627370497.5  // 4503499627370498
> (more exponentials as non-exponential)

Done.

floitsch

Committed patchset #7 manually as r23062 (presubmit successful).

7 years, 7 months ago (2013-05-23 12:00:56 UTC) #15

floitsch

Committed patchset #8 manually as r23066 (presubmit successful).

7 years, 7 months ago (2013-05-23 13:09:15 UTC) #17

floitsch

7 years, 7 months ago (2013-05-23 14:30:29 UTC) #18

Message was sent while issue was closed.

Committed patchset #10 manually as r23071 (presubmit successful).

Expand Messages | Collapse Messages

Issue 15333006: Rewrite double.parse. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix typo, fix test, and rebase. #

Patch Set 3 : Address comments. #

Patch Set 4 : Add parenthesis. #

Patch Set 5 : Add test for external strings. #

Patch Set 6 : Address comments. #

Patch Set 7 : Address comments #

Patch Set 8 : Fix test. #

Patch Set 9 : Updload #

Patch Set 10 : Upload #

Messages