Fix int.parse bug (dart2js version)

There was a weird dead expression that should have been used but was not, letting all upper case letters be 'accepted' for any radix over 10.  The result was NaN values flowing into the program.

R=lrn@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44304

[sra1, 2015-03-05 21:15:51]

sra@google.com changed reviewers:
+ lrn@google.com

[sra1, 2015-03-05 21:38:45]

Patchset #1 (id:1) has been deleted

[sra1, 2015-03-05 21:38:52]

Patchset #1 (id:20001) has been deleted

[Lasse Reichstein Nielsen, 2015-03-06 13:06:52]

lgtm

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:608: // again.
If \s doesn't always include U+0085, maybe change \s to [\s\x85] in the regexp?

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:625: throw new
RangeError("Radix $radix not in range 2..36");
You can change this one to:
  throw new RangeError.range(radix, 2, 36, "radix");

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:635: // and lower-cases ASCII
letters.
How about doing parseInt first, and only scanning the string if it seems to have
not accounted for all digits?

Maybe:

  var s = match[digitsIndex];
  // count digits
  int digits = s.length;
  // skip leading zeros.  
  // - or maybe just let anything with leading zeros go slow-case.
  for (int i = 0; i < s.length; i++) {
    if (s.codeUnitAt(i) == 0x30) {
      digits--;
    } else {
      break;
    }
  }
  var result = JS("num", "parseInt(#, #)", s, radix);
  if (result.isNaN) return _parseError(source, handleError);
  // The result is correct for a valid prefix of s. If that prefix is
  // all of s, then the result can be returned - that is, if all digits 
  // are used.
  // Math.pow should be logarithmic on integers.
  // (Could there be rounding issues?).
  if (result >= Math.pow(radix, digits)) return result;
  // Otherwise check digits.
  // Fallback in case of leading zeros (if not counted above) and 
  // maybe rounding errors?
  int digitLimit = radix < 10 ? 0x30 + radix : 0x61 - 10 + radix;
  for (int i = 0; i < s.length; i++) {
    if ((s.codeUnitAt(i) | 0x20) >= digitLimit) {
      return _parsError(source, handleError)
    }
  }
  return result;

Alternatively just do the digit check and then use parseInt "safely"?

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:641: maxCharCode = 0x30 + radix
- 1;
Maybe: 0x30 - 1 + radix
to constant fold (0x30-1) into 0x2f.
If the output is already 0x2f+radix, bonus points to the compiler.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:648: maxCharCode = 0x61 + radix
- 10 - 1;
Similar constant folding possible.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:659: return JS('int',
r'parseInt(#, #)', source, radix);
Make a comment that the checks above guarantee that all the digits are valid and
there are at least one of them, so parseInt can't return NaN.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/core/int.dart
File sdk/lib/core/int.dart (right):

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/core/int.dart#ne...
sdk/lib/core/int.dart:280: * The [onError] handler may return `null`.  This is
preferable to catching
The [onError] handler can be chosen to return `null`.
This is preferable to to throwing and then immediately catching the
[FormatException]. Example:

[sra1, 2015-03-06 21:34:13]

Committed patchset #3 (id:80001) manually as 44304 (presubmit successful).

[sra1, 2015-03-06 21:35:10]

Patchset #2 (id:60001) has been deleted

[sra1, 2015-03-06 21:38:39]

FYI I pulled the decimal match out of the matches since it is used on all paths.
 That manages to eliminate all but the first bounds check, reducing the
generated code from 66 lines to 37 lines.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right):

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:608: // again.
On 2015/03/06 13:06:51, Lasse Reichstein Nielsen wrote:
> If \s doesn't always include U+0085, maybe change \s to [\s\x85] in the
regexp?

In that case, JavaScript's parseInt fails too.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:625: throw new
RangeError("Radix $radix not in range 2..36");
On 2015/03/06 13:06:51, Lasse Reichstein Nielsen wrote:
> You can change this one to:
>   throw new RangeError.range(radix, 2, 36, "radix");

Done.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:635: // and lower-cases ASCII
letters.
On 2015/03/06 13:06:51, Lasse Reichstein Nielsen wrote:
> How about doing parseInt first, and only scanning the string if it seems to
have
> not accounted for all digits?
> 
> Maybe:
> 
>   var s = match[digitsIndex];
>   // count digits
>   int digits = s.length;
>   // skip leading zeros.  
>   // - or maybe just let anything with leading zeros go slow-case.
>   for (int i = 0; i < s.length; i++) {
>     if (s.codeUnitAt(i) == 0x30) {
>       digits--;
>     } else {
>       break;
>     }
>   }
>   var result = JS("num", "parseInt(#, #)", s, radix);
>   if (result.isNaN) return _parseError(source, handleError);
>   // The result is correct for a valid prefix of s. If that prefix is
>   // all of s, then the result can be returned - that is, if all digits 
>   // are used.
>   // Math.pow should be logarithmic on integers.
>   // (Could there be rounding issues?).
>   if (result >= Math.pow(radix, digits)) return result;
>   // Otherwise check digits.
>   // Fallback in case of leading zeros (if not counted above) and 
>   // maybe rounding errors?
>   int digitLimit = radix < 10 ? 0x30 + radix : 0x61 - 10 + radix;
>   for (int i = 0; i < s.length; i++) {
>     if ((s.codeUnitAt(i) | 0x20) >= digitLimit) {
>       return _parsError(source, handleError)
>     }
>   }
>   return result;
> 
> Alternatively just do the digit check and then use parseInt "safely"?

Another idea is to have a table of per-radix regexps.  They will scan faster
than anything we can write by hand.

The downside of these ideas is that it is more code.  Everything in this method
is a tax over JavaScript.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:641: maxCharCode = 0x30 + radix
- 1;
On 2015/03/06 13:06:51, Lasse Reichstein Nielsen wrote:
> Maybe: 0x30 - 1 + radix
> to constant fold (0x30-1) into 0x2f.
> If the output is already 0x2f+radix, bonus points to the compiler.

Done.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:648: maxCharCode = 0x61 + radix
- 10 - 1;
On 2015/03/06 13:06:51, Lasse Reichstein Nielsen wrote:
> Similar constant folding possible.

Done.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/js_lib/js_helper.dart:659: return JS('int',
r'parseInt(#, #)', source, radix);
On 2015/03/06 13:06:51, Lasse Reichstein Nielsen wrote:
> Make a comment that the checks above guarantee that all the digits are valid
and
> there are at least one of them, so parseInt can't return NaN.

Done.

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/core/int.dart
File sdk/lib/core/int.dart (right):

https://codereview.chromium.org/988523002/diff/40001/sdk/lib/core/int.dart#ne...
sdk/lib/core/int.dart:280: * The [onError] handler may return `null`.  This is
preferable to catching
On 2015/03/06 13:06:52, Lasse Reichstein Nielsen wrote:
> The [onError] handler can be chosen to return `null`.
> This is preferable to to throwing and then immediately catching the
> [FormatException]. Example:

Done.