- Implement a first cut for const String.env in the VM to allow

- Implement a first cut for const String.env in the VM to allow
  for configurations. int.env and bool.env to follow.
- Reintroduce native const factory constructors.

[Ivan Posva, 2013-09-27 07:21:01]

Re-animated a 6+months old patch. This allows setting configurations in the VM
as a basis for the rest of the implementation.
Still need to figure out how to add tests.

-Ivan

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:128: * Returns the string for the given environment
variable [name] or
Maybe it should say "for the given configuration variable"?

[kasperl, 2013-09-27 07:32:43]

Looking forward to seeing this stuff land!

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:128: * Returns the string for the given environment
variable [name] or
Nit: I don't think we should limit ourselves to environment variables. Also I'd
prefer a non-abbreviated name for this.

Ideally, the variables you can refer to can be defined by the embedder so we can
have them put in script tags in Dartium or passed on the command line for the
standalone VM and dart2js.

[bakster, 2013-09-27 07:44:39]

- Consider default values for 'defaultValue'.
- Please rename env to fromEnvironment.
  env is an abbreviation and it is not descriptive. 
Otherwise LGTM.

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

https://codereview.chromium.org/24975002/diff/1/runtime/bin/main.cc#newcode437
runtime/bin/main.cc:437: UNIMPLEMENTED();
Why not implement int and bool when you are at it?

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

https://codereview.chromium.org/24975002/diff/1/runtime/lib/integers_patch.da...
runtime/lib/integers_patch.dart:90: /* patch */ const factory int.env(String
name, int defaultValue)
[int defaultValue = 0]

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

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/int.dart#newcode256
sdk/lib/core/int.dart:256: /**
Too long lines.

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:131: external const String.env(String name, String
defaultValue);
How about adding a default value to defaultValue?
  external const String.env(String name, [String defaultValue=''] );

[ahe, 2013-09-27 07:57:00]

What does this solve?

[floitsch, 2013-09-27 13:52:31]

LGTM.
I don't have a better name than "fromEnvironment" although I agree with Kasper
that "environment" might not be a perfect fit. I could live with it, though.

https://codereview.chromium.org/24975002/diff/1/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/24975002/diff/1/runtime/lib/string_patch.dart...
runtime/lib/string_patch.dart:10: /* patch */ const factory String.env(String
name, String defaultValue)
[String defaultValue = ""]

[Ivan Posva, 2013-09-27 21:05:05]

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

https://codereview.chromium.org/24975002/diff/1/runtime/bin/main.cc#newcode437
runtime/bin/main.cc:437: UNIMPLEMENTED();
On 2013/09/27 07:44:39, bakster wrote:
> Why not implement int and bool when you are at it?

Because I wanted to get feedback on the initial approach before proceeding on
filling in these blanks. Seems that there is agreement to proceed (modulo the
naming).

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

https://codereview.chromium.org/24975002/diff/1/runtime/lib/integers_patch.da...
runtime/lib/integers_patch.dart:90: /* patch */ const factory int.env(String
name, int defaultValue)
On 2013/09/27 07:44:39, bakster wrote:
> [int defaultValue = 0]

See comment in string.dart.

https://codereview.chromium.org/24975002/diff/1/runtime/lib/string_patch.dart
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/24975002/diff/1/runtime/lib/string_patch.dart...
runtime/lib/string_patch.dart:10: /* patch */ const factory String.env(String
name, String defaultValue)
On 2013/09/27 13:52:31, floitsch wrote:
> [String defaultValue = ""]

ditto.

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

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/int.dart#newcode256
sdk/lib/core/int.dart:256: /**
On 2013/09/27 07:44:39, bakster wrote:
> Too long lines.

Thanks for noticing. Comments will need to be updated to match the naming in any
case.

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:128: * Returns the string for the given environment
variable [name] or
On 2013/09/27 07:32:43, kasperl wrote:
> Nit: I don't think we should limit ourselves to environment variables. Also
I'd
> prefer a non-abbreviated name for this.
> 
> Ideally, the variables you can refer to can be defined by the embedder so we
can
> have them put in script tags in Dartium or passed on the command line for the
> standalone VM and dart2js.

That is why had the remark about changing the comment. What we need to decide is
what we call this feature. When I initially wrote this patch in March, the plan
was to call this configuration. Any suggestions as far as naming goes are
appreciated.

I'll continue with env until we settled on a better plan. The other suggestion
the reviews namely "fromEnvironment" is also too specific.

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:131: external const String.env(String name, String
defaultValue);
On 2013/09/27 07:44:39, bakster wrote:
> How about adding a default value to defaultValue?
>   external const String.env(String name, [String defaultValue=''] );

I was thinking the same thing, but instead I would leave the defaults null,
allowing the program to figure out whether a value had been specified or not:

external const String.env(String name, {String default = null});

[Lasse Reichstein Nielsen, 2013-09-28 00:13:38]

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

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/int.dart#newcode259
sdk/lib/core/int.dart:259: * integer literal a [FormatException] is thrown.
Consider reformatting as:

  The integer value of the environment value for the key `name`.
  
  If [name] is not an environment key, instead returns [defaultValue].

  Throws a [FormatException] if the environment value is not an
  integer literal.

I try to avoid "returns" since it is a constructor, but that is what it does
with [defaultValue]. That suggests that it is an odd kind of constructor - more
like a factory constructor. Maybe mark it as such, if it has to be a constructor
at all.

https://codereview.chromium.org/24975002/diff/1/sdk/lib/core/int.dart#newcode261
sdk/lib/core/int.dart:261: external int.env(String name, int defaultValue);
Why is this a constructor instead of a static function?

I seem to remember the idea was to make it a const constructor (so the  value
would be a const value), but in that case, it can't throw.

[ahe, 2013-09-30 05:25:51]

lgtm

What is a "configuration"? Can you give an example of how you plan to use this?

Sometimes people have used the word configurations to deal with importing
depending on external parameters. I'd say that const constructors on String and
int aren't going to solve that, so I'm confused as to what the purpose of this
CL is.

The reason why I don't see const constructors as a solution to the configuration
problem is that you need to determine what is in scope before you start
evaluating const expressions:

import 'mylib_${const String.fromEnvironment("OS_NAME")}.dart';
import 'dart:core' as core; // Whoops: String isn't in scope.

[ahe, 2013-09-30 05:26:54]

Sorry, didn't mean to say "LGTM", accidentally hit the wrong button on my phone.

[Sean Eagan, 2013-10-01 16:58:25]

On 2013/09/30 05:25:51, ahe wrote:
> lgtm
> 
> What is a "configuration"? Can you give an example of how you plan to use
this?
> 
> Sometimes people have used the word configurations to deal with importing
> depending on external parameters. I'd say that const constructors on String
and
> int aren't going to solve that, so I'm confused as to what the purpose of this
> CL is.
> 
> The reason why I don't see const constructors as a solution to the
configuration
> problem is that you need to determine what is in scope before you start
> evaluating const expressions:
> 
> import 'mylib_${const String.fromEnvironment("OS_NAME")}.dart';
> import 'dart:core' as core; // Whoops: String isn't in scope.

DBC

Would it be possible to first resolve a scope using only the imports/exports
which don't use String interpolation, then use that to evaluate the
imports/exports which do use String interpolation, and extend the scope?

The most important configuration has to be standalone vs. browser, which this CL
doesn't seem to tackle.  Unless it is planned to just be a predefined
configuration variable:

export '${const bool.env('IS_BROWSER') ? 'browser' : 'standalone'}.dart';

but I think a dedicated library would be better:

import 'dart:env' as env;
export '${env.isBrowser ? 'browser' : 'standalone'}.dart';

[floitsch, 2013-10-01 17:20:44]

This has nothing to do with configuration specific imports.

I agree with Lasse: the constructors need to be `const` and therefore can't
throw.

[Lasse Reichstein Nielsen, 2013-10-10 07:45:02]

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

https://codereview.chromium.org/24975002/diff/31001/sdk/lib/core/int.dart#new...
sdk/lib/core/int.dart:257: * Returns the integer value for the given environment
variable [name] or [defaultValue]
Long lines.

https://codereview.chromium.org/24975002/diff/31001/sdk/lib/core/int.dart#new...
sdk/lib/core/int.dart:258: * if [name] is not present. If the value of the
environment variable is not a valid
I'd prefer to use defaultValue if the environment variable exists, but is not a
valid integer? Seems more consistent, since it's the way to avoid a
formatException that you can't check for.

It's not unprecedented that at const constructor throws (q.v. Symbol), but it's
still a little iffy (then it's a *compile time error* to do 'const int("x")' if
the env contains a value for "x" that isn't an integer).

Try making the first sentence shorter (base operation, no exceptions), and
elaborate in a second paragraph. Something like:

/**
 * Returns the integer value with the name [name] in the system
 * environment.
 *
 * If the environment does not contain an entry called [name],
 * the [defaultValue] is returned instead.
 * 
 * Throws a [FormatException] if the environment contains an entry
 * called `name` with a value that isn't an integer value.
 */

https://codereview.chromium.org/24975002/diff/31001/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/24975002/diff/31001/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:129: * [defaultValue] if [name] is not present.
We haven't specified what an "environment variable" is (or even an
"environment"). In a browser, it's not obvious that one exists, or if it does,
that its values are always strings.

That is, I think we should elaborate something like:

 An "environment" is a mapping from string "names" to string values
 that may be provided by the surrounding system. The environment
 may change between runs of the same program, but doesn't change 
 during a single execution.

maybe surrounding -> embedding, or is that too technical?

Is environment per-isolate? Can you change the environment when launching a new
isolate (preferably, yes)?

https://codereview.chromium.org/24975002/diff/31001/tests/corelib/string_env_...
File tests/corelib/string_env_test.dart (right):

https://codereview.chromium.org/24975002/diff/31001/tests/corelib/string_env_...
tests/corelib/string_env_test.dart:9: const String.env("NOT_IN_ENV",
defaultValue:"theDefault"));
Indentation off by one.

[ahe, 2013-10-10 14:09:09]

Adding Brian for a heads up on compile-time constant evaluation.

[rakudrama, 2013-10-10 15:48:29]

How is this supposed to work with ahead-of-time compilation like dart2js?

I would want to use the same collection of configuration values with programs
deployed using VM and dart2js.
If the process environment is used as the collection, how do you tell if an
envvar is a configuration for the dart2js compiler or the compiland?

[ahe, 2013-10-10 15:52:05]

On 2013/10/10 15:48:29, rakudrama wrote:
> How is this supposed to work with ahead-of-time compilation like dart2js?
> 
> I would want to use the same collection of configuration values with programs
> deployed using VM and dart2js.
> If the process environment is used as the collection, how do you tell if an
> envvar is a configuration for the dart2js compiler or the compiland?

In this context, my understanding is that the word "environment" is a little
more general than environment variables. My understanding is that these values
will be specified on the command line similar to -D options to gcc.

[Lasse Reichstein Nielsen, 2013-10-11 07:21:35]

On 2013/10/10 15:52:05, ahe wrote:
> On 2013/10/10 15:48:29, rakudrama wrote:
> > How is this supposed to work with ahead-of-time compilation like dart2js?
> > 
> > I would want to use the same collection of configuration values with
programs
> > deployed using VM and dart2js.
> > If the process environment is used as the collection, how do you tell if an
> > envvar is a configuration for the dart2js compiler or the compiland?
> 
> In this context, my understanding is that the word "environment" is a little
> more general than environment variables. My understanding is that these values
> will be specified on the command line similar to -D options to gcc.

The VM running in Dartium will use the "environment" of the browser (whatever
that may be) for const String.env's, because ... well, it's all it has.

If compiling the same program with dart2js will resolve constant environment
values at compile time, it will give a different result. Ofcourse, if running JS
doesn't have any environment to access, there won't be any other way to do it.

If we allow modifying the environment in new isolates, dart2js may need to do
environment resolution at runtime as well.

[Ivan Posva, 2013-10-11 16:22:10]

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

https://codereview.chromium.org/24975002/diff/31001/sdk/lib/core/int.dart#new...
sdk/lib/core/int.dart:257: * Returns the integer value for the given environment
variable [name] or [defaultValue]
On 2013/10/10 07:45:03, Lasse Reichstein Nielsen wrote:
> Long lines.

You are commenting on a revision that was only uploaded as a checkpoint to move
between machines. Thanks for you suggestions nonetheless.

-Ivan