Implement is-checks against type variables.

Implement is-checks against type variables.

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

[ahe, 2013-02-14 16:58:33]

DBC.

Awesome, I only looked at some of the files, but spotted a few nits.

https://chromiumcodereview.appspot.com/12210142/diff/4006/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://chromiumcodereview.appspot.com/12210142/diff/4006/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2454:
ClassElement currentClass = enclosingElement.getEnclosingClass();
I think this is shadowing a field (which contains the same value).

https://chromiumcodereview.appspot.com/12210142/diff/4006/tests/language/gene...
File tests/language/generic2_test.dart (right):

https://chromiumcodereview.appspot.com/12210142/diff/4006/tests/language/gene...
tests/language/generic2_test.dart:4: // Dart test program for testing the
instanceof operation.
Add line between copyright and comment.

Also, what is the "instanceof" operation?

https://chromiumcodereview.appspot.com/12210142/diff/4006/tests/language/gene...
tests/language/generic2_test.dart:16: Expect.isFalse(new A<int>().foo('hest'));
Why are you not testing B here? for example:

Expect.isTrue(new A<B>().foo(new B()));
Expect.isFalse(new A<B>().foo(new Object()));
Expect.isFalse(new A<B>().foo(1));

[karlklose, 2013-02-18 08:57:40]

Adding Johnni.

[Johnni Winther, 2013-02-18 09:16:27]

lgtm

https://chromiumcodereview.appspot.com/12210142/diff/4006/tests/language/gene...
File tests/language/generic2_test.dart (right):

https://chromiumcodereview.appspot.com/12210142/diff/4006/tests/language/gene...
tests/language/generic2_test.dart:17: }
I would like to see tests of on a subtype of A. For instance if class C<T>
extends A<T> and class D extends A<int>, then to have new C<int>().foo(...) and
new D().foo(...) tests.

[ngeoffray, 2013-02-18 09:27:57]

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:139: // Find
all instantiated types that are a supertype of a class that uses
supertype -> subtype?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:145: for
(classes = compiler.world.classesUsingTypeVariableTests;
Shouldn't that be in codegenWorld as well?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:840: if (other
== compiler.objectClass && classElement != other) return;
Why this change? It looks like you're getting rid of $isA on the A class.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:902: void
emitRuntimeClassesAndTests(CodeBuffer buffer) {
should that be called emitRuntimeTypesAndTests? What's a runtime class?

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:31:
compiler.stringClass];
What happened to the array class? And what about DOM classes?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:38:
String getNativeCheckName() => r'$nativeCheck';
Move this to the namer?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:45: *
Construct the native check function for the class [element].
Construct -> Constructs

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:54:
js.Expression getNativeCheck(Element element) {
Instead of creating a function with hand-written JS, could we instead have a
method on the native types and let it do the type check there?

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1525: *
representation [t].
Is [t] the same thing as [t] in isSubtype?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1527: bool
objectIsSubtype(Object o, var t) {
Why isn't o.runtimeType sufficient?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1533: } else {
Why are you not calling isSubtype here? Can't you get the runtime type
representation of o?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2455:
compiler.world.registerTypeVariableCheck(currentClass);
Register in resolver world?

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2586: return
createForeign(code, HType.READABLE_ARRAY, inputs);
I'd remove this helper, it's only called once.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2647: if
(type.element.isTypeVariable()) {
Don't we have a check on the type to know if it's a type variable?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2651: HInstruction
runtimeType = representations[0];
Instead of asserting it's 1 and fetching the first element, have to methods for
buildTypeArgumentRepresentations: one for type variable, and one for the other
cases?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2658: instruction =
new HInvokeStatic(inputs, HType.UNKNOWN);
HType.BOOLEAN?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2659:
add(instruction);
Register in codegen world?

[karlklose, 2013-02-18 16:02:01]

Thanks for the comments!

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:139: // Find
all instantiated types that are a supertype of a class that uses
On 2013/02/18 09:27:58, ngeoffray wrote:
> supertype -> subtype?

Done.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:145: for
(classes = compiler.world.classesUsingTypeVariableTests;
This is in the shared world. I am not sure it is worth it to move it to
resolution and codegen universes.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:840: if (other
== compiler.objectClass && classElement != other) return;
We may need the $isObject flag on objects now, when a type A<Object> tests
against its type variable. This installs Object.$isObject, if it is necessary,
but returns for C.$isObject for all classes C != Object.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:902: void
emitRuntimeClassesAndTests(CodeBuffer buffer) {
Initially I called the holder object runtime-classes. Would
'emitRuntimeTypeSupport' be better?

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:31:
compiler.stringClass];
Having list here was not necessary in the first place, it needs no holder
object.  These natives are only datatypes native to JS, which we cannot install
checks on.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:38:
String getNativeCheckName() => r'$nativeCheck';
On 2013/02/18 09:27:58, ngeoffray wrote:
> Move this to the namer?

Done.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:45: *
Construct the native check function for the class [element].
On 2013/02/18 09:27:58, ngeoffray wrote:
> Construct -> Constructs

Done.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:54:
js.Expression getNativeCheck(Element element) {
Not at the moment, but I should perhaps move the tests, substitutions and
native-checks to the interceptors.

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1525: *
representation [t].
Yes, added a comment.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1527: bool
objectIsSubtype(Object o, var t) {
No, runtimeType will return a Type that is only useful for comparison. It does
not support subtype checks.
(Additionally, it turns the runtimeType support on, which is not free ATM.)

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1533: } else {
I could, but not directly, I would have to construct it; I think that would be
more complicated.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2454:
ClassElement currentClass = enclosingElement.getEnclosingClass();
Done, removed.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2455:
compiler.world.registerTypeVariableCheck(currentClass);
Why? This is done to add the class to the set of classes needing rti and to note
the fact that it uses its argument in a check.  The is-check is already
registered in l. 2445 above.

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2586: return
createForeign(code, HType.READABLE_ARRAY, inputs);
On 2013/02/18 09:27:58, ngeoffray wrote:
> I'd remove this helper, it's only called once.

Done.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2647: if
(type.element.isTypeVariable()) {
No, we only have these helpers on elements. Replaced it with a test of the
TypeKind.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2651: HInstruction
runtimeType = representations[0];
On 2013/02/18 09:27:58, ngeoffray wrote:
> Instead of asserting it's 1 and fetching the first element, have to methods
for
> buildTypeArgumentRepresentations: one for type variable, and one for the other
> cases?

Done.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2658: instruction =
new HInvokeStatic(inputs, HType.UNKNOWN);
On 2013/02/18 09:27:58, ngeoffray wrote:
> HType.BOOLEAN?

Done.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2659:
add(instruction);
On 2013/02/18 09:27:58, ngeoffray wrote:
> Register in codegen world?

Done.

https://codereview.chromium.org/12210142/diff/4006/tests/language/generic2_te...
File tests/language/generic2_test.dart (right):

https://codereview.chromium.org/12210142/diff/4006/tests/language/generic2_te...
tests/language/generic2_test.dart:4: // Dart test program for testing the
instanceof operation.
On 2013/02/14 16:58:33, ahe wrote:
> Add line between copyright and comment.
> 
> Also, what is the "instanceof" operation?

Done.

https://codereview.chromium.org/12210142/diff/4006/tests/language/generic2_te...
tests/language/generic2_test.dart:16: Expect.isFalse(new A<int>().foo('hest'));
On 2013/02/14 16:58:33, ahe wrote:
> Why are you not testing B here? for example:
> 
> Expect.isTrue(new A<B>().foo(new B()));
> Expect.isFalse(new A<B>().foo(new Object()));
> Expect.isFalse(new A<B>().foo(1));

Done.

https://codereview.chromium.org/12210142/diff/4006/tests/language/generic2_te...
tests/language/generic2_test.dart:17: }
On 2013/02/18 09:16:27, Johnni Winther wrote:
> I would like to see tests of on a subtype of A. For instance if class C<T>
> extends A<T> and class D extends A<int>, then to have new C<int>().foo(...)
and
> new D().foo(...) tests.

Done.

[karlklose, 2013-02-19 08:57:38]

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

https://codereview.chromium.org/12210142/diff/16001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1125: if
(other == compiler.objectClass && classElement != other) return;
Actually we do not need to emit it, because we generate the native check for it.

[ngeoffray, 2013-02-19 09:00:41]

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:145: for
(classes = compiler.world.classesUsingTypeVariableTests;
On 2013/02/18 16:02:01, karlklose wrote:
> This is in the shared world. I am not sure it is worth it to move it to
> resolution and codegen universes.

Why not? The codegen world will have less or equal number of classes using type
variable tests.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:840: if (other
== compiler.objectClass && classElement != other) return;
On 2013/02/18 16:02:01, karlklose wrote:
> We may need the $isObject flag on objects now, when a type A<Object> tests
> against its type variable. This installs Object.$isObject, if it is necessary,
> but returns for C.$isObject for all classes C != Object.

So why did you remove it in the new version of the CL?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:902: void
emitRuntimeClassesAndTests(CodeBuffer buffer) {
On 2013/02/18 16:02:01, karlklose wrote:
> Initially I called the holder object runtime-classes. Would
> 'emitRuntimeTypeSupport' be better?

+1

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:31:
compiler.stringClass];
On 2013/02/18 16:02:01, karlklose wrote:
> Having list here was not necessary in the first place, it needs no holder
> object.  These natives are only datatypes native to JS, which we cannot
install
> checks on.

Please add this as a comment.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:54:
js.Expression getNativeCheck(Element element) {
On 2013/02/18 16:02:01, karlklose wrote:
> Not at the moment, but I should perhaps move the tests, substitutions and
> native-checks to the interceptors.

Yes, please add a TODO or file a bug. We don't want this hand-generated type
tests to be duplicated.

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1533: } else {
On 2013/02/18 16:02:01, karlklose wrote:
> I could, but not directly, I would have to construct it; I think that would be
> more complicated.

Why would you have to construct it? Isn't it set when the object is being
allocated? If not, don't you already have helpers to create it?

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2455:
compiler.world.registerTypeVariableCheck(currentClass);
On 2013/02/18 16:02:01, karlklose wrote:
> Why? This is done to add the class to the set of classes needing rti and to
note
> the fact that it uses its argument in a check.  The is-check is already
> registered in l. 2445 above.

If codegen doesn't need to know what classes make type variable check, I would
not register it. If emitter requires to know, I would have the codegen register
it in the codegen's world.

The "global" world is something we'll try to rip off a bit, and it should not
contain backend specific things. Collecting the classes that do type variable
check is required just because of the JS backend. So actually, this is something
the codegen should register to the js_backend.

https://codereview.chromium.org/12210142/diff/19001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12210142/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:993: if
(!type.isRaw || type is TypeVariableType) {
Banned is check :)

Do you really need all the following just because we have (a is T) ? What about
cases where we never allocate generic things.

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

https://codereview.chromium.org/12210142/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1619: if
(compiler.world.classesUsingTypeVariableTests.contains(superclass)) {
Why is this needed now? Please add a comment and/or example.

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

https://codereview.chromium.org/12210142/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2633:
compiler.enqueuer.codegen.registerIsCheck(type);
Do you need this? Won't the static helper enqueue what is needed already?

Also, we should not enqueue in the builder, but in the codegen.

[karlklose, 2013-02-19 12:39:28]

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:145: for
(classes = compiler.world.classesUsingTypeVariableTests;
Moved to the backend.

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:31:
compiler.stringClass];
On 2013/02/19 09:00:41, ngeoffray wrote:
> On 2013/02/18 16:02:01, karlklose wrote:
> > Having list here was not necessary in the first place, it needs no holder
> > object.  These natives are only datatypes native to JS, which we cannot
> install
> > checks on.
> 
> Please add this as a comment.

Done.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:54:
js.Expression getNativeCheck(Element element) {
On 2013/02/19 09:00:41, ngeoffray wrote:
> On 2013/02/18 16:02:01, karlklose wrote:
> > Not at the moment, but I should perhaps move the tests, substitutions and
> > native-checks to the interceptors.
> 
> Yes, please add a TODO or file a bug. We don't want this hand-generated type
> tests to be duplicated.

Done.

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

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1533: } else {
On 2013/02/18 09:27:58, ngeoffray wrote:
> Why are you not calling isSubtype here? Can't you get the runtime type
> representation of o?

Done.

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12210142/diff/4006/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2455:
compiler.world.registerTypeVariableCheck(currentClass);
I moved this set to the backend.

[ngeoffray, 2013-02-21 10:26:18]

Last round of comments, (small) things are still a bit unclear to me.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:665:
Iterable<ClassElement> cachedClassesUsingTypeVariableTests;
Should that be in the emitter instead? It would like very dangerous to call it
before we are done compiling methods.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1009: if
(!type.isRaw || type is TypeVariableType) {
use type.kind instead?

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1014:
enqueueInResolution(getGetObjectIsSubtype());
Isn't that one just requires by TypeVariableType?

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:146: // set of
is-checks.
We have a [subtypes] set in the world. Couldn't you use that instead of
iterating over all instantiated types?

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:25: *
Contains the classes for which we use builtin JavaScript datatypes on which
on which we -> and therefore cannot

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:34:
compiler.stringClass];
Could you add a comment on why you don't put list here?

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:43:
return element == compiler.objectClass || isJsNative(element);
How about super types of string?

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:74: test
= js.strictEquals(variable, new jsAst.Binary('|', variable, zero));
Our int check is Math.floor(a) === a

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1555: var type = o;
Please add a comment on why o could be the type.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1557: if
(JS('bool', '# != null', rti)) {
Add a comment on why it could be null.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1574: *  2) a Dart
object.
That is very confusing, how can a Dart object be a type?

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2500: //
TODO(ngeoffray): Also handle T a (in checked mode).
Isn't that trivial to fix? Have the SSA builder treat T a jsut like it treats a
is T ?

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2635:
compiler.enqueuer.codegen.registerIsCheck(type);
Don't we register the is check in the SSA codegen already? If not, please add a
comment and a TODO that something is fishy here.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2653:
compiler.findHelper(const SourceString('objectIsSubtype'));
Use the method on the backend to get the helper.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2660: ClassElement
currentClass = currentElement.getEnclosingClass();
Remove currentClass.

[karlklose, 2013-02-21 14:48:44]

Thanks, Nicolas.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:665:
Iterable<ClassElement> cachedClassesUsingTypeVariableTests;
On 2013/02/21 10:26:18, ngeoffray wrote:
> Should that be in the emitter instead? It would like very dangerous to call it
> before we are done compiling methods.

Done.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1009: if
(!type.isRaw || type is TypeVariableType) {
On 2013/02/21 10:26:18, ngeoffray wrote:
> use type.kind instead?

Done.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1014:
enqueueInResolution(getGetObjectIsSubtype());
On 2013/02/21 10:26:18, ngeoffray wrote:
> Isn't that one just requires by TypeVariableType?

Done.

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:146: // set of
is-checks.
No, we want to have the instantiated types to now what type arguments are used;
typesImplementedBySubclasses collects classes.

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:25: *
Contains the classes for which we use builtin JavaScript datatypes on which
On 2013/02/21 10:26:18, ngeoffray wrote:
> on which we -> and therefore cannot

Done.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:34:
compiler.stringClass];
On 2013/02/21 10:26:18, ngeoffray wrote:
> Could you add a comment on why you don't put list here?

Done.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:43:
return element == compiler.objectClass || isJsNative(element);
Which do we have?

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:74: test
= js.strictEquals(variable, new jsAst.Binary('|', variable, zero));
On 2013/02/21 10:26:18, ngeoffray wrote:
> Our int check is Math.floor(a) === a

Done.

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1555: var type = o;
On 2013/02/21 10:26:18, ngeoffray wrote:
> Please add a comment on why o could be the type.

Done.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1557: if
(JS('bool', '# != null', rti)) {
On 2013/02/21 10:26:18, ngeoffray wrote:
> Add a comment on why it could be null.

Done.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1574: *  2) a Dart
object.
As discussed, the checks and substitutions are on its prototype so it serves as
a representation of its class.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2500: //
TODO(ngeoffray): Also handle T a (in checked mode).
It probably is, but I don't want to do that in this CL.

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2635:
compiler.enqueuer.codegen.registerIsCheck(type);
It does not for type variable tests, because they do not generate an HIs. I
moved the call to the type variable branch.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2653:
compiler.findHelper(const SourceString('objectIsSubtype'));
On 2013/02/21 10:26:18, ngeoffray wrote:
> Use the method on the backend to get the helper.

Done.

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2660: ClassElement
currentClass = currentElement.getEnclosingClass();
On 2013/02/21 10:26:18, ngeoffray wrote:
> Remove currentClass.

Done.

[ngeoffray, 2013-02-21 16:31:55]

LGTM!

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

https://codereview.chromium.org/12210142/diff/22001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:43:
return element == compiler.objectClass || isJsNative(element);
On 2013/02/21 14:48:44, karlklose wrote:
> Which do we have?

Comparable<String>, Pattern.

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

https://codereview.chromium.org/12210142/diff/29001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:28: *
need a native check.
Why doesn't need a native check? It's implemented with a builtin JavaScript
datatype.

https://codereview.chromium.org/12210142/diff/29001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:75: test
= new jsAst.LiteralExpression.withData(
There are better ways to do this. Please take a look at emitter.dart, isInt.

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

https://codereview.chromium.org/12210142/diff/29001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1558: var type = o;
I would move this assignment and the comment in the else branch of the if line
1563.

https://codereview.chromium.org/12210142/diff/29001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1562: // compound
type representation, otherwise use the [o] itself.
use the -> use

[karlklose, 2013-02-22 10:51:49]

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

https://codereview.chromium.org/12210142/diff/29001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:28: *
need a native check.
That was actually a bug. I added handling for lists in the new patch set.

https://codereview.chromium.org/12210142/diff/29001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:75: test
= new jsAst.LiteralExpression.withData(
Done, I will convert the rest of the js-code in a follow up.

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

https://codereview.chromium.org/12210142/diff/29001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1558: var type = o;
On 2013/02/21 16:31:55, ngeoffray wrote:
> I would move this assignment and the comment in the else branch of the if line
> 1563.

Done.

https://codereview.chromium.org/12210142/diff/29001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1562: // compound
type representation, otherwise use the [o] itself.
On 2013/02/21 16:31:55, ngeoffray wrote:
> use the -> use

Done.

[ngeoffray, 2013-02-22 11:04:30]

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

https://codereview.chromium.org/12210142/diff/23012/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:45:
return element == compiler.objectClass || isJsNative(element);
I don't think you answered my question about supertypes of String (and now
List). Don't you need a native check when doing:

var c = new A<Iterable>();
c.foo([]);

class A<T> {
  foo(a) => a is T;
}

Please add tests for those types.

https://codereview.chromium.org/12210142/diff/23012/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12210142/diff/23012/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:89: ClassElement enclosing
= variable.enclosingElement;
Why this change?

https://codereview.chromium.org/12210142/diff/23012/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:120:
compiler.hasVariableTypecheck;
So any class needs rti when the check is on? Shouldn't we track the classes used
in a type variable, and the class that declares the type variable does a type
variable is check?

[karlklose, 2013-02-26 11:18:50]

Please take another look, Nicolas. I changed the code to use the interceptors
for JS-native values and I now register instantiated abstract classes as needing
rti.

[ngeoffray, 2013-02-26 14:11:37]

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:103:
compiler.world.registerInstantiatedAbstractClass(cls);
Instead of doing this here, it looks a bit cleaner to add factory constructors.
So the check could be whenever you enqueue a factory constructor of an abstract
class.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:104:
compiler.backend.registerRuntimeType();
You should not register this here.

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

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:135:
checkedTypedefs == null);
Fits in one line.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1093: if
(other == compiler.objectClass && other != classElement) return;
Why did you add it back now? This deserves a comment or an abstraction.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1158: bool
needsHolder(ClassElement cls) {
Please add a comment on who needs a holder.

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

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1551: var rti =
getRuntimeTypeInfo(o);
Create rti closer to where it is used.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1552: if
(isJsArray(o) || o is int || o is String || o is double) {
How about calling getInterceptor unconditionally and checking if what you got
back is the ObjectInterceptor (soon, you won't have to check at all,
getInterceptor would return you the object if o is not a primitive).

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1562: // compound
type representation, otherwise use [o] itself.
The 'otherhwise' part applies to the else. Please move it there.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:93: * Compute
type arguments of classes that potentially use their type
Why potentially? We know they are for the resolution universe, as well as for
the codegen universe.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:102: if
(!classesUsingChecks.isEmpty) {
How about:
if (classesUsingChecks.isEmpty) return;

Also, please add a comment on why you're doing this check.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (left):

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:63: if
(cls.typeVariables.isEmpty) return;
Why are you removing this check?

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:13: final Set<ClassElement>
instantiatedAbstractClasses;
This name makes it think like it's an error. Maybe call it
usedFactoriesOfAbstractClasses and store the factories instead?

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:85: if (type is
TypeVariableType) {
Use the kind?

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:92: if (type is
InterfaceType) {
Use the kind?

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:97: } else if (type is
TypeVariableType) {
USe the kind?

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:105: // information before
adding the class.
Isn't that in the rtiDependencies map? Thinking about it, why don't we register
a dependency between the abstract class and the instantiated class? The abstract
class would then automatically be needing rti.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:135:
compiler.enabledRuntimeType;
Fits in one line.

[karlklose, 2013-02-27 10:12:58]

Thanks, Nicolas!

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:103:
compiler.world.registerInstantiatedAbstractClass(cls);
I moved the code to addToWorklist.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:104:
compiler.backend.registerRuntimeType();
Removed. Fixed a bug with registering getRuntimeTypeArgument correclty in the
resolver now.

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

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:135:
checkedTypedefs == null);
On 2013/02/26 14:11:37, ngeoffray wrote:
> Fits in one line.

Done.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1093: if
(other == compiler.objectClass && other != classElement) return;
We need Object.$isObject. Comment added.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1158: bool
needsHolder(ClassElement cls) {
On 2013/02/26 14:11:37, ngeoffray wrote:
> Please add a comment on who needs a holder.

Done.

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

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1551: var rti =
getRuntimeTypeInfo(o);
o can potentially be replaced by the intercecptor, which does not have the rti.
That woud be wrong for lists.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1552: if
(isJsArray(o) || o is int || o is String || o is double) {
On 2013/02/26 14:11:37, ngeoffray wrote:
> How about calling getInterceptor unconditionally and checking if what you got
> back is the ObjectInterceptor (soon, you won't have to check at all,
> getInterceptor would return you the object if o is not a primitive).

Done.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1562: // compound
type representation, otherwise use [o] itself.
On 2013/02/26 14:11:37, ngeoffray wrote:
> The 'otherhwise' part applies to the else. Please move it there.

Done.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:93: * Compute
type arguments of classes that potentially use their type
I meant it may use it, if the method containing it is called. rephrased.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:102: if
(!classesUsingChecks.isEmpty) {
On 2013/02/26 14:11:37, ngeoffray wrote:
> How about:
> if (classesUsingChecks.isEmpty) return;
> 
> Also, please add a comment on why you're doing this check.

Done.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (left):

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:63: if
(cls.typeVariables.isEmpty) return;
That was an error. Restored.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:13: final Set<ClassElement>
instantiatedAbstractClasses;
On 2013/02/26 14:11:37, ngeoffray wrote:
> This name makes it think like it's an error. Maybe call it
> usedFactoriesOfAbstractClasses and store the factories instead?

Done.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:85: if (type is
TypeVariableType) {
On 2013/02/26 14:11:37, ngeoffray wrote:
> Use the kind?

Done.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:92: if (type is
InterfaceType) {
On 2013/02/26 14:11:37, ngeoffray wrote:
> Use the kind?

Done.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:97: } else if (type is
TypeVariableType) {
On 2013/02/26 14:11:37, ngeoffray wrote:
> USe the kind?

Done.

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:105: // information before
adding the class.
How do we know the instantiated class (in general)?

https://codereview.chromium.org/12210142/diff/34002/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:135:
compiler.enabledRuntimeType;
On 2013/02/26 14:11:37, ngeoffray wrote:
> Fits in one line.

Done.

[ngeoffray, 2013-02-27 12:36:28]

A few last comments, I'm not sure about why you're registering things to the
backend so eagerly.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:70:
compiler.backend.registerSetRuntimeType();
This (registerSetRuntimeType) should be done elsewhere, when we realize the type
arguments are actually needed.

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

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1094: // Avoid
emitting [:$isObject:] on all classes but [Object].
But will we? We're not adding anything on a class that the super class. So maybe
this check is not useful?

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1095: return;
Remove tabs.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2433:
compiler.backend.registerGetRuntimeTypeArgument();
Why? If there is no is check, there is no need for runtimeTypeArgument.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:102: if
(classesUsingChecks.isEmpty) return;
Add a comment?

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:112: // We need
the type as instance of its superclass anyway, so we just
Remove tabs.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:13: final Set<ClassElement>
usedFactoriesOfAbstractClasses;
Store the factories instead, as implied by the name?

[karlklose, 2013-02-27 16:11:32]

Thanks Nicolas. As discussed I added a special dependency for the JS backend
instead of tracking factories.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:70:
compiler.backend.registerSetRuntimeType();
Obsolete.

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

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1094: // Avoid
emitting [:$isObject:] on all classes but [Object].
We need this check, since we do not track the is-checks emitted on object (we
could ignore Object completely before, since all checks were statically known.)

But I plan on revising computation and emitting of these checks and
substitutions anyway.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1095: return;
On 2013/02/27 12:36:28, ngeoffray wrote:
> Remove tabs.

Done.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2433:
compiler.backend.registerGetRuntimeTypeArgument();
Obsolete.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:102: if
(classesUsingChecks.isEmpty) return;
On 2013/02/27 12:36:28, ngeoffray wrote:
> Add a comment?

Done.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:112: // We need
the type as instance of its superclass anyway, so we just
On 2013/02/27 12:36:28, ngeoffray wrote:
> Remove tabs.

Done.

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12210142/diff/26038/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:13: final Set<ClassElement>
usedFactoriesOfAbstractClasses;
Obsolete.

[ngeoffray, 2013-02-27 16:18:37]

LGTM!

https://codereview.chromium.org/12210142/diff/23016/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/12210142/diff/23016/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:107: // TODO(karlklose):
get rid of this and specify the dependencies in the patch.
What's the patch? Please expand the comment and explain what you would need to
do.

https://codereview.chromium.org/12210142/diff/23016/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12210142/diff/23016/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:56:
compiler.backend.addBackendRtiDependencies(this);
Explain why you're doing this, and why now.

[karlklose, 2013-02-28 11:37:46]

https://codereview.chromium.org/12210142/diff/23016/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/12210142/diff/23016/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:107: // TODO(karlklose):
get rid of this and specify the dependencies in the patch.
On 2013/02/27 16:18:37, ngeoffray wrote:
> What's the patch? Please expand the comment and explain what you would need to
> do.

Done.

https://codereview.chromium.org/12210142/diff/23016/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12210142/diff/23016/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:56:
compiler.backend.addBackendRtiDependencies(this);
On 2013/02/27 16:18:37, ngeoffray wrote:
> Explain why you're doing this, and why now.

Done.

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

https://codereview.chromium.org/12210142/diff/42001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:66: //
TODO(karlklose): remove this temporary fix: we need to add the classes
I want to fix this in the next CL.

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

https://codereview.chromium.org/12210142/diff/42001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1610: // The class
of [s] is a subclass of the class of [t].  If [s] has no type
This is the bug I talked with you about.

[ngeoffray, 2013-02-28 12:14:51]

LGTM

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

https://codereview.chromium.org/12210142/diff/42001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1538:
getArguments(var type) => isJsArray(type) ? JS('var', r'#.slice(1)', type) :
null;
Line too long. Why this change? What code is affected?

https://codereview.chromium.org/12210142/diff/42001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1610: // The class
of [s] is a subclass of the class of [t].  If [s] has no type
On 2013/02/28 11:37:46, karlklose wrote:
> This is the bug I talked with you about.

Did you add a test for this?

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

https://codereview.chromium.org/12210142/diff/49001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:67: //
used in substitutions to addArguments.
What's the implication of this? The comment is not very helping describing the
problem.

[karlklose, 2013-02-28 12:19:58]

Thanks again.

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

https://codereview.chromium.org/12210142/diff/42001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1538:
getArguments(var type) => isJsArray(type) ? JS('var', r'#.slice(1)', type) :
null;
Changed in the new version.

The change in isSubtype means that getArguments can be called on an object (like
$.String) given as first argument to isSubtype, because we have a substitution.
We have to either make getArguments work with these or not call it from
isSubtype. I decided to change it here to not make isSubtype  more complex.

https://codereview.chromium.org/12210142/diff/42001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1610: // The class
of [s] is a subclass of the class of [t].  If [s] has no type
I also fixed generics_native_test in this patch set (the last line was wrong)
and this triggered the bug.

[karlklose, 2013-02-28 12:20:46]

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

https://codereview.chromium.org/12210142/diff/49001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:67: //
used in substitutions to addArguments.
We will generate holders for these in every program.

[karlklose, 2013-02-28 12:23:32]

Committed patchset #18 manually as r19216 (presubmit successful).

[ngeoffray, 2013-02-28 12:39:03]

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

https://codereview.chromium.org/12210142/diff/49001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:67: //
used in substitutions to addArguments.
On 2013/02/28 12:20:46, karlklose wrote:
> We will generate holders for these in every program.

OK, please make sure fixing this is #1 in your TODO list, to keep hello world
tidy.