Implement fromEnvironment on bool, int and String

Implement fromEnvironment on bool, int and String

This implements const constructor fromEnvironment on bool, int and
String.

The VM have the added -Dname=value option to define the value for the
properties. All values are provided by using the -D - nothing is read
from the environment.

If the resulting value is null or - in the case of int.fromEnvironment
- not a number an ArgumentError is thrown.

This CL does not have any implementation for dart2js.

This is a continuation of the change
https://chromiumcodereview.appspot.com/24975002 by iposva@
BUG=
R=iposva@google.com

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

[Søren Gjesse, 2013-10-29 16:54:36]

There are still some outstanding questions:

Is ArgumentError the correct error?
Should we support hex numberx (0x...)?
Does the embedder need to information on the type requested?

[siva, 2013-10-29 20:50:22]

DBC.

How is this supposed to work in dartium?

I don't think these can be command line flags to chrome.
We would need some kind of meta tags and the general feeling is to not require
additional tags for Dart.

[Lasse Reichstein Nielsen, 2013-10-29 21:05:34]

On 2013/10/29 20:50:22, siva wrote:
> DBC.
> 
> How is this supposed to work in dartium?
> 
> I don't think these can be command line flags to chrome.
> We would need some kind of meta tags and the general feeling is to not require
> additional tags for Dart.

Meta-tags could be used. Or server response flags.
Alternatively, they could be extra parameters on the script URL.

But more likely, in the short run, there would be no user values at all in the
browser. If you need it, you can compile with dart2dart before sending the dart
code to the browser.

If we add system properties that should always be there, then the embedder can
make those available (perhaps nothing else than the equivalent of
"-Dbrowser=true").

[siva, 2013-10-29 21:23:41]

I find the design process to be completely broken were in  APIs which depend on
the embedder for implementation are being added to the libraries without first
considering the consequences on all the embedding situations.

[Ivan Posva, 2013-10-29 21:42:58]

Needs a lot more work.

-Ivan

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode76
runtime/bin/main.cc:76: static HashMap* configuration = NULL;
environment to match the renamed const factory constructor. Here and everywhere
else.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode147
runtime/bin/main.cc:147: return key == NULL ? 0 : *key;
Why bother with a hash table if there are only 26 likely hash values?  Just use
a linked list of entries, it is easier to understand and we do not expect to
have large numbers of entries here anyway.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode155
runtime/bin/main.cc:155: if (configuration == NULL) configuration = new
HashMap(&SameStringValue, 4);
if (...) {
  environment = ...
}

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode156
runtime/bin/main.cc:156: char* name;
Please add comments about what you are doing here. For example:
// Split the args at "=".

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode163
runtime/bin/main.cc:163: if (name_len == 0) return false;
if (...) {
  return false;
}

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode166
runtime/bin/main.cc:166: *(name + name_len) = '\0';
name[name_len] = '\0';

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode173
runtime/bin/main.cc:173: ASSERT(entry != NULL);
In a debug VM you will fail, but in a release VM you will just override. Just
define it to overwrite, like -D on a gcc command line and remove the assertion.
Alternatively if we do not want to allow overwriting, then you should return a
failure from here.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode174
runtime/bin/main.cc:174: entry->value = value != NULL ? value :
const_cast<char*>("");
(value != NULL) ?

https://codereview.chromium.org/50983002/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/50983002/diff/1/runtime/include/dart_api.h#ne...
runtime/include/dart_api.h:2111: * Configurations
Environment here and everywhere else.

Please see the updated CL which has been taken over in this review.

https://codereview.chromium.org/50983002/diff/1/runtime/lib/bool.cc
File runtime/lib/bool.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/lib/bool.cc#newcode31
runtime/lib/bool.cc:31: return (strcmp("false", chars) != 0)
Only "true" is the true value in Dart, you should follow this here as well. Also
this should be handled based on the Dart_EnvironmentType in the embedder. This
code here should be expecting a boolean from the embedder.

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

https://codereview.chromium.org/50983002/diff/1/runtime/lib/integers.cc#newco...
runtime/lib/integers.cc:246: // TODO(sgjesse): Support hex strings.
Integer::NewCanonical already deals with hex strings.

https://codereview.chromium.org/50983002/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/lib/string.cc#newcode24
runtime/lib/string.cc:24: Dart_Handle result = callback(kStringConfig,
Here and the other callbacks: You are ignoring all errors being returned from
the callback. If an error was returned, then we should probably have that error
surface as the argument error.

https://codereview.chromium.org/50983002/diff/1/runtime/lib/string.cc#newcode29
runtime/lib/string.cc:29: if (len > 0) {
The empty string should be a valid value from the environment.

https://codereview.chromium.org/50983002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:12378: RawInteger* Integer::NewCanonical(int64_t value) {
Please remove as this is not needed anywhere as far as I understand.

https://codereview.chromium.org/50983002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/50983002/diff/1/runtime/vm/object.h#newcode4364
runtime/vm/object.h:4364: static RawInteger* NewCanonical(int64_t value);
ditto

[Søren Gjesse, 2013-10-29 22:44:38]

An initial clarifying question

https://codereview.chromium.org/50983002/diff/1/runtime/lib/bool.cc
File runtime/lib/bool.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/lib/bool.cc#newcode31
runtime/lib/bool.cc:31: return (strcmp("false", chars) != 0)
On 2013/10/29 21:42:59, Ivan Posva wrote:
> Only "true" is the true value in Dart, you should follow this here as well.
Also
> this should be handled based on the Dart_EnvironmentType in the embedder. This
> code here should be expecting a boolean from the embedder.

One question here. I was a bit puzzeled about the kXXXConfig constants for the
embedder. Should the implementation be changed to expect

  For kIntegerConfig: Null or an Integer object
  For kStringConfig: Null or a String object
  For kBoolConfig: Null or a Bool object

Then the embedder returns Null to provide the default value and otherwise the
parsing of integer and boolean strings are left to the embedder. The VM library
will not have any logic there. The embedder supplies an object of the expected
type or Null.

I had that at some point, which is also why the overloaded version of
Integer::NewCanonical is still in the CL.

[Ivan Posva, 2013-10-30 05:08:20]

https://codereview.chromium.org/50983002/diff/1/runtime/lib/bool.cc
File runtime/lib/bool.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/lib/bool.cc#newcode31
runtime/lib/bool.cc:31: return (strcmp("false", chars) != 0)
On 2013/10/29 22:44:39, Søren Gjesse wrote:
> On 2013/10/29 21:42:59, Ivan Posva wrote:
> > Only "true" is the true value in Dart, you should follow this here as well.
> Also
> > this should be handled based on the Dart_EnvironmentType in the embedder.
This
> > code here should be expecting a boolean from the embedder.
> 
> One question here. I was a bit puzzeled about the kXXXConfig constants for the
> embedder. Should the implementation be changed to expect
> 
>   For kIntegerConfig: Null or an Integer object
>   For kStringConfig: Null or a String object
>   For kBoolConfig: Null or a Bool object
> 
> Then the embedder returns Null to provide the default value and otherwise the
> parsing of integer and boolean strings are left to the embedder. The VM
library
> will not have any logic there. The embedder supplies an object of the expected
> type or Null.
> 
> I had that at some point, which is also why the overloaded version of
> Integer::NewCanonical is still in the CL.

I guess it would make the native API simpler and less cumbersome to use if the
callback was only ever expected to return a Dart_Null, a Dart_Error or a
Dart_String.

This was one of the open areas which I had not been happy with in my version of
the CL yet.

[Søren Gjesse, 2013-10-30 11:44:01]

PTAL

Addressed all comments.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode76
runtime/bin/main.cc:76: static HashMap* configuration = NULL;
On 2013/10/29 21:42:59, Ivan Posva wrote:
> environment to match the renamed const factory constructor. Here and
everywhere
> else.

Done.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode147
runtime/bin/main.cc:147: return key == NULL ? 0 : *key;
On 2013/10/29 21:42:59, Ivan Posva wrote:
> Why bother with a hash table if there are only 26 likely hash values?  Just
use
> a linked list of entries, it is easier to understand and we do not expect to
> have large numbers of entries here anyway.

Added a real hash function to platform/hashmap.h and used that.

I can't see why a linked list would be better. The hash map implementation is
already there.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode155
runtime/bin/main.cc:155: if (configuration == NULL) configuration = new
HashMap(&SameStringValue, 4);
On 2013/10/29 21:42:59, Ivan Posva wrote:
> if (...) {
>   environment = ...
> }

Done.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode156
runtime/bin/main.cc:156: char* name;
On 2013/10/29 21:42:59, Ivan Posva wrote:
> Please add comments about what you are doing here. For example:
> // Split the args at "=".

Done.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode163
runtime/bin/main.cc:163: if (name_len == 0) return false;
On 2013/10/29 21:42:59, Ivan Posva wrote:
> if (...) {
>   return false;
> }

Done.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode166
runtime/bin/main.cc:166: *(name + name_len) = '\0';
On 2013/10/29 21:42:59, Ivan Posva wrote:
> name[name_len] = '\0';

Done.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode173
runtime/bin/main.cc:173: ASSERT(entry != NULL);
On 2013/10/29 21:42:59, Ivan Posva wrote:
> In a debug VM you will fail, but in a release VM you will just override. Just
> define it to overwrite, like -D on a gcc command line and remove the
assertion.
> Alternatively if we do not want to allow overwriting, then you should return a
> failure from here.

When Lookup is called with the last argument set to true it will allocate a new
entry if it is not found. Therefore the ASSERT. Overwriting works fine. I will
add a test for overwrite.

https://codereview.chromium.org/50983002/diff/1/runtime/bin/main.cc#newcode174
runtime/bin/main.cc:174: entry->value = value != NULL ? value :
const_cast<char*>("");
On 2013/10/29 21:42:59, Ivan Posva wrote:
> (value != NULL) ?

That was wrong. A non-existent value is now passed as NULL.

https://codereview.chromium.org/50983002/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/50983002/diff/1/runtime/include/dart_api.h#ne...
runtime/include/dart_api.h:2111: * Configurations
On 2013/10/29 21:42:59, Ivan Posva wrote:
> Environment here and everywhere else.
> 
> Please see the updated CL which has been taken over in this review.

Done.

https://codereview.chromium.org/50983002/diff/1/runtime/lib/bool.cc
File runtime/lib/bool.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/lib/bool.cc#newcode31
runtime/lib/bool.cc:31: return (strcmp("false", chars) != 0)
On 2013/10/30 05:08:21, Ivan Posva wrote:
> On 2013/10/29 22:44:39, Søren Gjesse wrote:
> > On 2013/10/29 21:42:59, Ivan Posva wrote:
> > > Only "true" is the true value in Dart, you should follow this here as
well.
> > Also
> > > this should be handled based on the Dart_EnvironmentType in the embedder.
> This
> > > code here should be expecting a boolean from the embedder.
> > 
> > One question here. I was a bit puzzeled about the kXXXConfig constants for
the
> > embedder. Should the implementation be changed to expect
> > 
> >   For kIntegerConfig: Null or an Integer object
> >   For kStringConfig: Null or a String object
> >   For kBoolConfig: Null or a Bool object
> > 
> > Then the embedder returns Null to provide the default value and otherwise
the
> > parsing of integer and boolean strings are left to the embedder. The VM
> library
> > will not have any logic there. The embedder supplies an object of the
expected
> > type or Null.
> > 
> > I had that at some point, which is also why the overloaded version of
> > Integer::NewCanonical is still in the CL.
> 
> I guess it would make the native API simpler and less cumbersome to use if the
> callback was only ever expected to return a Dart_Null, a Dart_Error or a
> Dart_String.
> 
> This was one of the open areas which I had not been happy with in my version
of
> the CL yet.

Changed so that only "true" is true.

Changed the API to no longer have the type given to the callback and updated the
documentation to say return either a string or null.

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

https://codereview.chromium.org/50983002/diff/1/runtime/lib/integers.cc#newco...
runtime/lib/integers.cc:246: // TODO(sgjesse): Support hex strings.
On 2013/10/29 21:42:59, Ivan Posva wrote:
> Integer::NewCanonical already deals with hex strings.

Changed checking to check for decimal or hexdecimal string before calling
Integer::NewCanonical.

https://codereview.chromium.org/50983002/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/lib/string.cc#newcode24
runtime/lib/string.cc:24: Dart_Handle result = callback(kStringConfig,
On 2013/10/29 21:42:59, Ivan Posva wrote:
> Here and the other callbacks: You are ignoring all errors being returned from
> the callback. If an error was returned, then we should probably have that
error
> surface as the argument error.

Done.

If the embedder returns an error object I now throw an argument error like this:

  Exceptions::ThrowArgumentError(
      String::Handle(
          String::New(Error::Cast(error).ToErrorCString())));

I would have liked to throw an internal error instead, like this:

  const Array& exception_arguments = Array::Handle(Array::New(1));
  exception_arguments.SetAt(
      0, Object::Handle(String::New(Error::Cast(error).ToErrorCString())));
  Exceptions::ThrowByType(Exceptions::kInternalError, exception_arguments);

However that hit an assert in the VM:

  runtime/vm/dart_entry.cc:306: error: expected: !cls.IsNull()
  Aborted (core dumped)

Another option will be to just abort like this:

  Exceptions::PropagateError(Error::Cast(error));

I think ArgumentError is OK.

https://codereview.chromium.org/50983002/diff/1/runtime/lib/string.cc#newcode29
runtime/lib/string.cc:29: if (len > 0) {
On 2013/10/29 21:42:59, Ivan Posva wrote:
> The empty string should be a valid value from the environment.

Absolutely. Removed the check.

https://codereview.chromium.org/50983002/diff/1/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/50983002/diff/1/runtime/vm/object.cc#newcode1...
runtime/vm/object.cc:12378: RawInteger* Integer::NewCanonical(int64_t value) {
On 2013/10/29 21:42:59, Ivan Posva wrote:
> Please remove as this is not needed anywhere as far as I understand.

Done.

https://codereview.chromium.org/50983002/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/50983002/diff/1/runtime/vm/object.h#newcode4364
runtime/vm/object.h:4364: static RawInteger* NewCanonical(int64_t value);
On 2013/10/29 21:42:59, Ivan Posva wrote:
> ditto

Done.

[kasperl, 2013-10-30 13:32:48]

https://codereview.chromium.org/50983002/diff/230001/sdk/lib/core/bool.dart
File sdk/lib/core/bool.dart (right):

https://codereview.chromium.org/50983002/diff/230001/sdk/lib/core/bool.dart#n...
sdk/lib/core/bool.dart:17: * [defaultValue] if [name] is not present.
Shouldn't this actually have a defaultValue parameter? It is mentioned in the
comment and it does make sense even for booleans.

[kasperl, 2013-10-30 13:47:02]

So what's the status of this when it comes to wrong values from the environment?
If I have a property "foo" set in the environment with an non-int value and I
do:

   const int.fromEnvironment("foo", defaultValue: 100)

it seems unpleasant if I just get 100 back. There are no static warnings to help
you so getting a good runtime error seems much better to me. I think it will be
hard for users to figure out that they mistyped something in a value. 

I understand that users will always get the default value if they mistype the
property name which definitely also sucks. I wonder if there's a reasonable way
of helping our users. If we don't do anything about it, there's a good chance
that this will be just like a script that ignores unrecognized options by
default. Not good.

[Søren Gjesse, 2013-10-30 13:59:29]

https://codereview.chromium.org/50983002/diff/230001/sdk/lib/core/bool.dart
File sdk/lib/core/bool.dart (right):

https://codereview.chromium.org/50983002/diff/230001/sdk/lib/core/bool.dart#n...
sdk/lib/core/bool.dart:17: * [defaultValue] if [name] is not present.
On 2013/10/30 13:32:49, kasperl wrote:
> Shouldn't this actually have a defaultValue parameter? It is mentioned in the
> comment and it does make sense even for booleans.

In the latest design we removed the defaultValue for bool.fromEnvironment. It
seemed to have little value.

[kasperl, 2013-10-30 14:05:23]

On 2013/10/30 13:59:29, Søren Gjesse wrote:
> https://codereview.chromium.org/50983002/diff/230001/sdk/lib/core/bool.dart
> File sdk/lib/core/bool.dart (right):
> 
>
https://codereview.chromium.org/50983002/diff/230001/sdk/lib/core/bool.dart#n...
> sdk/lib/core/bool.dart:17: * [defaultValue] if [name] is not present.
> On 2013/10/30 13:32:49, kasperl wrote:
> > Shouldn't this actually have a defaultValue parameter? It is mentioned in
the
> > comment and it does make sense even for booleans.
> 
> In the latest design we removed the defaultValue for bool.fromEnvironment. It
> seemed to have little value.

Why? I think it would be very convenient to be able to flip a 'logging' flag on
while testing my stuff without having to modify the command line or anything
like it. It may have less value than for the other types but for consistency I
would definitely want it to be there.

[Lasse Reichstein Nielsen, 2013-10-30 18:28:55]

> I understand that users will always get the default value if they mistype the
> property name which definitely also sucks. I wonder if there's a reasonable
way
> of helping our users. If we don't do anything about it, there's a good chance
> that this will be just like a script that ignores unrecognized options by
> default. Not good.

For that I recommend adding (at a later time) a check for the existence of a 
property, e.g., const bool.inEnvironment(String name).

[Søren Gjesse, 2013-10-30 20:21:48]

On 2013/10/30 13:47:02, kasperl wrote:
> So what's the status of this when it comes to wrong values from the
environment?
> If I have a property "foo" set in the environment with an non-int value and I
> do:
> 
>    const int.fromEnvironment("foo", defaultValue: 100)
> 
> it seems unpleasant if I just get 100 back. There are no static warnings to
help
> you so getting a good runtime error seems much better to me. I think it will
be
> hard for users to figure out that they mistyped something in a value. 
> 
> I understand that users will always get the default value if they mistype the
> property name which definitely also sucks. I wonder if there's a reasonable
way
> of helping our users. If we don't do anything about it, there's a good chance
> that this will be just like a script that ignores unrecognized options by
> default. Not good.

So in the case of int.fromEnvironment we can easily make it a compile time error
if
the value cannot be parsed as an integer instead of returning the default value.
Originally it was implemented that way, after re-reading the spec I changed it.
Wouldn't
a compile time error in this case make sense?

[Søren Gjesse, 2013-10-30 20:28:54]

On 2013/10/30 18:28:55, Lasse Reichstein Nielsen wrote:
> > I understand that users will always get the default value if they mistype
the
> > property name which definitely also sucks. I wonder if there's a reasonable
> way
> > of helping our users. If we don't do anything about it, there's a good
chance
> > that this will be just like a script that ignores unrecognized options by
> > default. Not good.
> 
> For that I recommend adding (at a later time) a check for the existence of a 
> property, e.g., const bool.inEnvironment(String name).

One can check for existence like this:

  bool isXInEnv = const String.fromEnvironment("x") != null;

there is no way null can emerge if the the name is defined (-Dx= is the empty
string).

[Ivan Posva, 2013-10-30 20:44:42]

-Ivan

https://codereview.chromium.org/50983002/diff/400001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/50983002/diff/400001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:76: // The configuration provided through the command line
using -D options.
Please change all mentions of "configuration" or "Config" to environment in the
whole CL.

https://codereview.chromium.org/50983002/diff/400001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:491: char* name_chars =
reinterpret_cast<char*>(malloc(utf8_len + 1));
LEAK

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

https://codereview.chromium.org/50983002/diff/400001/runtime/lib/integers.cc#...
runtime/lib/integers.cc:252: Dart_StringToCString(result, &digits);
You will have a problem here if the string contains non ASCII characters since
digits_len will not match what Dart_StringToCString will return. I suggest to
use Dart_StringToUTF8 in this case.

https://codereview.chromium.org/50983002/diff/400001/runtime/lib/integers.cc#...
runtime/lib/integers.cc:273: if (digits_len > 0 && is_number) {
Isn't digits_len > 0 guaranteed at this point?

https://codereview.chromium.org/50983002/diff/400001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/50983002/diff/400001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:36: }
if (Dart_IsString(result)) {
  ...
} else if (!Dart_IsNull(result)) {
  ...
}

as in Integer_fromEnvironment seems more readable to me.

[Søren Gjesse, 2013-10-30 21:24:33]

Addressed comments. Also added freeing of environment at shutdown.

https://codereview.chromium.org/50983002/diff/400001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/50983002/diff/400001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:76: // The configuration provided through the command line
using -D options.
On 2013/10/30 20:44:43, Ivan Posva wrote:
> Please change all mentions of "configuration" or "Config" to environment in
the
> whole CL.

Done. That should be the last, the raw patch file does not contain "config"
anymore.

https://codereview.chromium.org/50983002/diff/400001/runtime/bin/main.cc#newc...
runtime/bin/main.cc:491: char* name_chars =
reinterpret_cast<char*>(malloc(utf8_len + 1));
On 2013/10/30 20:44:43, Ivan Posva wrote:
> LEAK

Done.

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

https://codereview.chromium.org/50983002/diff/400001/runtime/lib/integers.cc#...
runtime/lib/integers.cc:252: Dart_StringToCString(result, &digits);
On 2013/10/30 20:44:43, Ivan Posva wrote:
> You will have a problem here if the string contains non ASCII characters since
> digits_len will not match what Dart_StringToCString will return. I suggest to
> use Dart_StringToUTF8 in this case.

Good catch. Fixed.

https://codereview.chromium.org/50983002/diff/400001/runtime/lib/integers.cc#...
runtime/lib/integers.cc:273: if (digits_len > 0 && is_number) {
On 2013/10/30 20:44:43, Ivan Posva wrote:
> Isn't digits_len > 0 guaranteed at this point?

No, in case of leading "-" the length is decremented above (these is a test with
-Db=-.

https://codereview.chromium.org/50983002/diff/400001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/50983002/diff/400001/runtime/lib/string.cc#ne...
runtime/lib/string.cc:36: }
On 2013/10/30 20:44:43, Ivan Posva wrote:
> if (Dart_IsString(result)) {
>   ...
> } else if (!Dart_IsNull(result)) {
>   ...
> }
> 
> as in Integer_fromEnvironment seems more readable to me.

Done. Included the IsNull case, and changed for all three files.

[Ivan Posva, 2013-10-31 05:32:59]

LGTM for the VM changes.

-Ivan

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

https://codereview.chromium.org/50983002/diff/510002/runtime/lib/integers.cc#...
runtime/lib/integers.cc:252: if (*digits == '-') {
digits[0] == '-' to be consistent with the use below.

[Søren Gjesse, 2013-10-31 05:47:22]

Committed patchset #16 manually as r29642 (presubmit successful).

[Lasse Reichstein Nielsen, 2013-10-31 06:55:37]

https://codereview.chromium.org/50983002/diff/1290001/sdk/lib/core/bool.dart
File sdk/lib/core/bool.dart (right):

https://codereview.chromium.org/50983002/diff/1290001/sdk/lib/core/bool.dart#...
sdk/lib/core/bool.dart:19: external const factory bool.fromEnvironment(String
name, {bool defaultValue});
I still think the default is silly, but if it is there, it should at least
default to false, not null.
The most common use should be to check for the presence of a -Dfoo=true, and you
don't want null there.

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

https://codereview.chromium.org/50983002/diff/1290001/sdk/lib/core/int.dart#n...
sdk/lib/core/int.dart:256: /**
Comments need reformatting. I'll look at it today.

[Søren Gjesse, 2013-10-31 07:05:10]

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

https://codereview.chromium.org/50983002/diff/510002/runtime/lib/integers.cc#...
runtime/lib/integers.cc:252: if (*digits == '-') {
On 2013/10/31 05:33:00, Ivan Posva wrote:
> digits[0] == '-' to be consistent with the use below.

Done.

[Lasse Reichstein Nielsen, 2013-10-31 07:15:30]

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

https://codereview.chromium.org/50983002/diff/510002/runtime/lib/integers.cc#...
runtime/lib/integers.cc:252: if (*digits == '-') {
This does not allow leading whitespace or a leading '+', which int.parse does.
We should match int.parse with no radix supplied.