Change int.fromEnvironment in VM to use same code as int.parse.

Change int.fromEnvironment in VM to use same code as int.parse.

R=fschneider@google.com, sgjesse@google.com

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

[Søren Gjesse, 2013-10-31 13:17:49]

lgtm, with comments

https://codereview.chromium.org/54663002/diff/40001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/54663002/diff/40001/runtime/lib/integers.cc#n...
runtime/lib/integers.cc:249: if (result.IsInteger()) return result.raw();
I think you need to go whrough return Integer::NewCanonical() here to ensure
non-smi values are canonicalized, e.g.

Integer::NewCanonical(String::Handle(String::New(result.ToCString())));

You can skip it is it's a smi.

https://codereview.chromium.org/54663002/diff/40001/runtime/lib/integers.cc#n...
runtime/lib/integers.cc:250: } else if (Dart_IsError(response)) {
Looks better with the sting case first and error/null after. Could you change
that for string and bool as well?

https://codereview.chromium.org/54663002/diff/40001/sdk/lib/_internal/lib/cor...
File sdk/lib/_internal/lib/core_patch.dart (right):

https://codereview.chromium.org/54663002/diff/40001/sdk/lib/_internal/lib/cor...
sdk/lib/_internal/lib/core_patch.dart:108: 'int.fromEnvironment can only be used
as a const constructor');
Thanks.

[Florian Schneider, 2013-10-31 13:23:10]

lgtm

[Lasse Reichstein Nielsen, 2013-10-31 13:25:49]

https://codereview.chromium.org/54663002/diff/40001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/54663002/diff/40001/runtime/lib/integers.cc#n...
runtime/lib/integers.cc:249: if (result.IsInteger()) return result.raw();
Will do. Florian, is there no simpler way?

https://codereview.chromium.org/54663002/diff/40001/runtime/lib/integers.cc#n...
runtime/lib/integers.cc:250: } else if (Dart_IsError(response)) {
Will do.

[Lasse Reichstein Nielsen, 2013-10-31 13:45:24]

Address comments.

https://codereview.chromium.org/54663002/diff/140001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/54663002/diff/140001/runtime/lib/integers.cc#...
runtime/lib/integers.cc:252: String::New(result.ToCString())));
Ivan, we can't decide if this canonicalization is still necessary.
If not, we can just return result.raw in all cases.

[Lasse Reichstein Nielsen, 2013-10-31 13:45:51]

Committed patchset #5 manually as r29670 (presubmit successful).

[Ivan Posva, 2013-10-31 16:08:17]

https://codereview.chromium.org/54663002/diff/140001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

https://codereview.chromium.org/54663002/diff/140001/runtime/lib/integers.cc#...
runtime/lib/integers.cc:250: if (result.IsSmi()) return result.raw();
{
  return result.raw();
}

https://codereview.chromium.org/54663002/diff/140001/runtime/lib/integers.cc#...
runtime/lib/integers.cc:252: String::New(result.ToCString())));
On 2013/10/31 13:45:24, Lasse Reichstein Nielsen wrote:
> Ivan, we can't decide if this canonicalization is still necessary.
> If not, we can just return result.raw in all cases.

Yes, it is necessary. You could of course do

return result.CheckAndCanonicalize();

to avoid parsing again.