Implement substitution for type variables.

Implement substitution for type variables.

This change does not yet address the case where there is more than one substitution, as in this example:

class S<X> {}
class S1 extends S<int> {}
class S2 extends S<double> {}
class T implements S1, S2 {}

main() {
  new T() is S<int>;
  new T() is S<double>;
}

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

[kasperl, 2013-01-28 14:13:32]

Karl, can you add a few pieces of generated JS code that use substitutions to
the CL to illustrate how this works?

[karlklose, 2013-01-29 13:38:20]

Here is a small example:

class A<T> {}
class B<U, V> {}
class C<T> extends A<B<A<T>, C<T>>> {}

main() {
  print(new C<int>() is A<B<A<num>, C<num>>>);
}

The generated class bodies with the substitution for C are:

$$.A = {"": "Object;", $isA: true};
$$.C = {"": "A;", $asA: function (T) { return [[$.B, [$.A, T], [$.C, T]]]; }};

And the typecheck looks like this:

  $.print(!!t1.$isA && $.checkArguments(t1.$asA, t2, [[$.B, [$.A, $.num], [$.C,
$.num]]]));

[kasperl, 2013-01-29 15:02:34]

Maybe you should have Johnni take a look at this too? He might be interested in
learning more about the runtime type information encoding.

https://codereview.chromium.org/12018015/diff/5001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12018015/diff/5001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1479: if
(!isJsArray(s)) throw 's not an array';
Turn these into asserts?

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:853:
buffer.add('$holder.${namer.substitutionName(check)}$_=${_}$body$N');
Isn't it a little bit dangerous to manipulate (add a suffix) to the name
produced by the namer? Wouldn't it be better to ask the namer to produce a
slightly different name? I'm concerned about minification, etc.j

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:38: //
Collect all types used in type arguments of instantiated types.
You could factor out the computation of these two sets in two helper functions.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:143: //
TODO(karlklose); handle closures.
; -> :

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:148: if
(type == null) return true;
Add a comment that explains why it's correct to return true here and in the next
case. Feel free to keep it short.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:153:
Link<DartType> variables = cls.typeVariables;
Explain what you're looking for in this loop in a few sentences in a comment.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:159:
identicalOrEmptyVariables = false;
Wouldn't it be cleaner just to return false here? I would try to get rid of the
identicalOrEmptyVariables variable.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:192:
target = target.asInstanceOf(check);
Move the asInstanceOf call to the line where you initialize target?

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:216: }
else {
I would probably split the case that has arguments out to get a single test of
the hasArguments variable (instead of two). I know that requires you to
duplicate the builder.add(getJsName(element)) part, but you could cache the
getJsName(element) result in a local variable.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1493: if
(!isJsArray(s)) throw 's not an array';
Change to asserts?

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1506: var
getArguments(var type) => JS('var', r'#.slice(1)', type);
var isn't a legal return type.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1508: var
getField(var object, String name) => JS('var', r'#[#]', object, name);
var isn't a legal return type.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:22: 
wat

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2553: return '#';
wat? This isn't a List<HInstruction>.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2615:
createForeign('#.$substitution', 'Object', [expression]);
Should the namer know about $substitution?

[karlklose, 2013-01-30 12:01:19]

Thanks for the comments, Kasper.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:853:
buffer.add('$holder.${namer.substitutionName(check)}$_=${_}$body$N');
I am using the name as returned by the namer, the $_ adds a space in unminified
mode.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:38: //
Collect all types used in type arguments of instantiated types.
On 2013/01/29 15:02:34, kasperl wrote:
> You could factor out the computation of these two sets in two helper
functions.

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:143: //
TODO(karlklose); handle closures.
On 2013/01/29 15:02:34, kasperl wrote:
> ; -> :

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:148: if
(type == null) return true;
On 2013/01/29 15:02:34, kasperl wrote:
> Add a comment that explains why it's correct to return true here and in the
next
> case. Feel free to keep it short.

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:153:
Link<DartType> variables = cls.typeVariables;
On 2013/01/29 15:02:34, kasperl wrote:
> Explain what you're looking for in this loop in a few sentences in a comment.

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:159:
identicalOrEmptyVariables = false;
On 2013/01/29 15:02:34, kasperl wrote:
> Wouldn't it be cleaner just to return false here? I would try to get rid of
the
> identicalOrEmptyVariables variable.

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:192:
target = target.asInstanceOf(check);
The problem here is that asInstanceOf is declared on InterfaceType, not on
DartType. So these are actually two type assertions, and I changed it to two
variables.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:216: }
else {
On 2013/01/29 15:02:34, kasperl wrote:
> I would probably split the case that has arguments out to get a single test of
> the hasArguments variable (instead of two). I know that requires you to
> duplicate the builder.add(getJsName(element)) part, but you could cache the
> getJsName(element) result in a local variable.

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1493: if
(!isJsArray(s)) throw 's not an array';
On 2013/01/29 15:02:34, kasperl wrote:
> Change to asserts?

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1506: var
getArguments(var type) => JS('var', r'#.slice(1)', type);
On 2013/01/29 15:02:34, kasperl wrote:
> var isn't a legal return type.

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1508: var
getField(var object, String name) => JS('var', r'#[#]', object, name);
On 2013/01/29 15:02:34, kasperl wrote:
> var isn't a legal return type.

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:22: 
Exactly. Removed.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2553: return '#';
This return belongs to the closure given to getTypeRepresentation.

The value is used to represent the type variable in the string that
getTypeRepresentation computes.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2615:
createForeign('#.$substitution', 'Object', [expression]);
It does, this is interpolating the name that we got from the namer in l.2613
into the template for the foreign instruction.

[ngeoffray, 2013-01-30 13:05:59]

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:843: // holder
object.
Is there a description of the "holder object"? Could you add one in this file?

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:847: if (check
== compiler.dynamicClass) {
Why is that necessary now?

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1215: if
(cls.superclass != null && checkedClasses.contains(cls.superclass)) {
Please add a comment here, and/or update the one line 1206.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:529: //
TODO(erikcorry): Reduce from is$x to ix when we are minifying.
is$x -> $isX

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:531: // needs to
know this name, because it uses a JS-call to read the field.
Why the TODO?

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:549: // needs to
know this name, because it uses a JS-call to read the field.
Ditto.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(left):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:72: if
(cls != compiler.dynamicClass && cls != compiler.objectClass) {
Why do you remove this?

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1470: return
JS('var', r'#.apply(null, #)', function, arguments);
"#(#)" ?

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1490: bool
areSubtypes(var s, var t) {
(var s, var t) -> (List s, List t)

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1542: if
(JS('bool', '# !== #', typeOfT, typeOfS)) {
Can we use Dart's "!=" for typeOfT and typeOfS?

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2552: return '#';
This seems like a very brittle dependency with getTypeRepresentation.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3150: return '#';
This seems like a very brittle dependency with the user of addTypeVariable.

[karlklose, 2013-01-30 15:37:18]

Thanks for the comments, Nicolas.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:843: // holder
object.
I added a short description comment to the mabeGenerateHolder function.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:847: if (check
== compiler.dynamicClass) {
It is not, good catch.  Removed.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1215: if
(cls.superclass != null && checkedClasses.contains(cls.superclass)) {
On 2013/01/30 13:06:00, ngeoffray wrote:
> Please add a comment here, and/or update the one line 1206.

Done.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:529: //
TODO(erikcorry): Reduce from is$x to ix when we are minifying.
On 2013/01/30 13:06:00, ngeoffray wrote:
> is$x -> $isX

Done.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:531: // needs to
know this name, because it uses a JS-call to read the field.
Leftover of the merge. Removed.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:549: // needs to
know this name, because it uses a JS-call to read the field.
On 2013/01/30 13:06:00, ngeoffray wrote:
> Ditto.

Done.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(left):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:72: if
(cls != compiler.dynamicClass && cls != compiler.objectClass) {
It is for the case where Object is used as an argument as in new G<Object>() is
G<B>, so we collect Object as instantiated type argument to have a target of the
$isB test.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1470: return
JS('var', r'#.apply(null, #)', function, arguments);
The arguments are in a JS array, but we want them to be passed as separate
arguments to the substitution function.

For example, the fucntion could be
 function (T, U) { return [$.Map, T, U]; }

and we would invoke it with arguments = [$.int, $.String].

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1490: bool
areSubtypes(var s, var t) {
On 2013/01/30 13:06:00, ngeoffray wrote:
> (var s, var t) -> (List s, List t)

Done.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1542: if
(JS('bool', '# !== #', typeOfT, typeOfS)) {
I don't think so, as it would try to call $eq on a JS function object.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2552: return '#';
getTypeRepresentation is a helper that does not know what to generate for type
variables, so we have to tell it. As an alternative I could make two copies of
getTypeRepresentation or give it a second function that provides the text for
the name. Do you prefer that?

[ngeoffray, 2013-01-31 08:28:08]

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2552: return '#';
On 2013/01/30 15:37:18, karlklose wrote:
> getTypeRepresentation is a helper that does not know what to generate for type
> variables, so we have to tell it. As an alternative I could make two copies of
> getTypeRepresentation or give it a second function that provides the text for
> the name. Do you prefer that?

Anything to remove this dependency yes. But what has changed since? You did not
need to pass the '#' before. getTypeRepresentation used to do it itself.

https://codereview.chromium.org/12018015/diff/14002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12018015/diff/14002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:833: * class
as a string (just like object constructors do).  The is-checkes
checkes -> checks

https://codereview.chromium.org/12018015/diff/14002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1212: * they
will be inherited at runtime, but we need to generate the substitution,
line too long

https://codereview.chromium.org/12018015/diff/14002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1213: *
because it may have changed.
Please provide an example.

[Johnni Winther, 2013-01-31 09:17:31]

LGTM (as much as I understand)

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:150: if
(check.typeVariables.isEmpty || cls == check) {
Move this before the asInstanceOf check.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:157:
assert(variables.head.kind == TypeKind.TYPE_VARIABLE);
Why this assertion? (It is true!)

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:165: if
(variables.isEmpty != arguments.isEmpty) {
If this is not the case it is an invariant breach on InterfaceType.

[karlklose, 2013-02-01 07:36:35]

Thanks for the comments, Johnni.

Please take another look at the emitter part. I had forgotten a case like the
following, where we have to overwrite inherited substitutions:
  class M<T, V> {}
  class A<T, V> implements M<V, T> {}
  class B<T, V> extends A<V, T> {}

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:150: if
(check.typeVariables.isEmpty || cls == check) {
On 2013/01/31 09:17:31, Johnni Winther wrote:
> Move this before the asInstanceOf check.

Done.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:157:
assert(variables.head.kind == TypeKind.TYPE_VARIABLE);
Removed.

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:165: if
(variables.isEmpty != arguments.isEmpty) {
I am not comparing type arguments to the same class here, so it may happen that
the length is different:
I compare cls.thisType.asInstanceOf(check).typeArguments with cls.typeVariables.

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12018015/diff/17001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2552: return '#';
The function is now also used in getSupertypeSubstitution, where it needs to
print the name of the function instead of a argument placeholder.

https://codereview.chromium.org/12018015/diff/14002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12018015/diff/14002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:833: * class
as a string (just like object constructors do).  The is-checkes
On 2013/01/31 08:28:08, ngeoffray wrote:
> checkes -> checks

Done.

https://codereview.chromium.org/12018015/diff/14002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1212: * they
will be inherited at runtime, but we need to generate the substitution,
On 2013/01/31 08:28:08, ngeoffray wrote:
> line too long

Done.

https://codereview.chromium.org/12018015/diff/14002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1213: *
because it may have changed.
On 2013/01/31 08:28:08, ngeoffray wrote:
> Please provide an example.

Done.

[karlklose, 2013-02-01 07:36:37]

Thanks for the comments, Johnni.

Please take another look at the emitter part. I had forgotten a case like the
following, where we have to overwrite inherited substitutions:
  class M<T, V> {}
  class A<T, V> implements M<V, T> {}
  class B<T, V> extends A<V, T> {}

[karlklose, 2013-02-01 07:36:39]

Thanks for the comments, Johnni.

Please take another look at the emitter part. I had forgotten a case like the
following, where we have to overwrite inherited substitutions:
  class M<T, V> {}
  class A<T, V> implements M<V, T> {}
  class B<T, V> extends A<V, T> {}

[Johnni Winther, 2013-02-01 07:54:46]

https://codereview.chromium.org/12018015/diff/9006/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12018015/diff/9006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:207:
print('$cls -> $check is trivial');
Remove debugging code.

https://codereview.chromium.org/12018015/diff/9006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:230:
onVariable(variable);
Why call onVariable when the result is not used?

[karlklose, 2013-02-01 07:58:46]

https://codereview.chromium.org/12018015/diff/9006/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12018015/diff/9006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:230:
onVariable(variable);
For side effects: The builder calls getTypeRepresentations with a callback that
creates instructions to retrieve the actual type argument from the object.

[Johnni Winther, 2013-02-01 08:09:11]

lgtm

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12018015/diff/9002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:165: if
(variables.isEmpty != arguments.isEmpty) {
On 2013/02/01 07:36:35, karlklose wrote:
> I am not comparing type arguments to the same class here, so it may happen
that
> the length is different:
> I compare cls.thisType.asInstanceOf(check).typeArguments with
cls.typeVariables.

Ahh!

https://codereview.chromium.org/12018015/diff/9006/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12018015/diff/9006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:230:
onVariable(variable);
On 2013/02/01 07:58:47, karlklose wrote:
> For side effects: The builder calls getTypeRepresentations with a callback
that
> creates instructions to retrieve the actual type argument from the object.

Add a comment. The locally visible use has no side effects.