Outline of how to factor out compiler and runtime specifics.

Outline of how to factor out compiler and runtime specifics.

Committed in one commit with https://codereview.chromium.org/179173003/.

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

[ahe, 2014-01-17 12:50:16]

This is an outline of a direction I want to take.

I'm about to add new compiler and runtime configurations, so I felt it was about
time to address this issue: making it easier to add compilers and runtimes.

[kustermann, 2014-01-17 13:00:53]

I think this is a good starting point.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
File dart/tools/testing/dart/runtime_configuration.dart (right):

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:33: throw "Unknown runtime
'$runtime'";
I don't like the fact that a class knows all of it's subclasses.
I'd prefer have a top level function which does the same thing.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_options.dart:715: new
CompilerConfiguration(configuration).computeTimeoutMultiplier(
I like how you've encapsulated this into methods on
CompilerConfiguration/RuntimeConfiguration.
Adding other methods for: constructing compiler flags based on TestInformation
objects ... is easily possible this way.

[kustermann, 2014-01-17 14:19:32]

rubberstamp lgtm

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/comp...
File dart/tools/testing/dart/compiler_configuration.dart (right):

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/comp...
dart/tools/testing/dart/compiler_configuration.dart:64: class
Dart2dartCompilerConfiguration extends Dart2xCompilerConfiguration {
Dart2dart => Dart2Dart ?

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
File dart/tools/testing/dart/runtime_configuration.dart (right):

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:28: case 'vm':
Just might want to use just a list:
NORMAL_RUNTIMES = ['d8', 'ff', ...];

if (NORMAL_RUNTIMES.contains(configuration['runtime']) return new
RuntimeConfiguration._subclass();

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:29: return new
RuntimeConfiguration._subclass();
The _subclass constructor is nowhere defined.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:33: throw "Unknown runtime
'$runtime'";
On 2014/01/17 13:00:53, kustermann wrote:
> I don't like the fact that a class knows all of it's subclasses.
> I'd prefer have a top level function which does the same thing.

Then you might be able to get rid of _subclass() as well.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:42: switch (arch) {
=> if (['simarm', 'simmips', ...].contains(arch)) {}

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/test...
File dart/tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/test...
dart/tools/testing/dart/test_options.dart:724: configuration['timeout'] = 60 *
compilerMulitiplier * runtimeMultiplier;
You're hopfully aware that this multiplication will make some timesouts larger
than they are now. Consider e.g. dart2js-drt-checked.

This makes tests that timeout have a potentially huge impact on cycle times. 
Please make sure, we don't change the semantics.

[ahe, 2014-01-17 16:39:58]

Thank you, Martin.

I plan to submit patch set 2 as is. But I want to find the right time to do so.
Then I'll iterate over the code in a number of incremental patches.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/comp...
File dart/tools/testing/dart/compiler_configuration.dart (right):

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/comp...
dart/tools/testing/dart/compiler_configuration.dart:64: class
Dart2dartCompilerConfiguration extends Dart2xCompilerConfiguration {
On 2014/01/17 14:19:33, kustermann wrote:
> Dart2dart => Dart2Dart ?

I believe the name of the product is dart2dart, so capitalized that is
Dart2dart.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
File dart/tools/testing/dart/runtime_configuration.dart (right):

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:28: case 'vm':
On 2014/01/17 14:19:33, kustermann wrote:
> Just might want to use just a list:
> NORMAL_RUNTIMES = ['d8', 'ff', ...];
> 
> if (NORMAL_RUNTIMES.contains(configuration['runtime']) return new
> RuntimeConfiguration._subclass();

I expect I'll have to add subclasses for most of the cases real soon.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:29: return new
RuntimeConfiguration._subclass();
On 2014/01/17 14:19:33, kustermann wrote:
> The _subclass constructor is nowhere defined.

Done.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:33: throw "Unknown runtime
'$runtime'";
On 2014/01/17 14:19:33, kustermann wrote:
> On 2014/01/17 13:00:53, kustermann wrote:
> > I don't like the fact that a class knows all of it's subclasses.
> > I'd prefer have a top level function which does the same thing.
> 
> Then you might be able to get rid of _subclass() as well.

What I would really like is to store these objects in [configuration].

In this case, I should be able to move all this code to test_options.dart.

I'd like to construct these objects without giving them the configuration map.

But I'm not exactly sure what direction to take, so I prefer to keep it as is,
until we have a better understanding.

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:42: switch (arch) {
On 2014/01/17 14:19:33, kustermann wrote:
> => if (['simarm', 'simmips', ...].contains(arch)) {}

That doesn't make any sense to me. That coding pattern is inherently inefficient
and also harder to read.

[kustermann, 2014-01-17 17:20:16]

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
File dart/tools/testing/dart/runtime_configuration.dart (right):

https://codereview.chromium.org/141713002/diff/1/dart/tools/testing/dart/runt...
dart/tools/testing/dart/runtime_configuration.dart:42: switch (arch) {
On 2014/01/17 16:39:58, ahe wrote:
> On 2014/01/17 14:19:33, kustermann wrote:
> > => if (['simarm', 'simmips', ...].contains(arch)) {}
> 
> That doesn't make any sense to me. That coding pattern is inherently
inefficient
> and also harder to read.

I think the coding pattern itself is not "inherently inefficient". How efficient
this code will be depends really on the implementation of the compiler.

The reason why I made the comment is because I think it's more readable, than
having a switch-case: it's more verbose, you've to think about the different
cases and the default case, is it fall-through or not. ['...'].contains(arch) is
easier for me.

But, feel free to keep it :)