Fix bug in type_variable_handler.

Move registration of type variable constants to resolution.

R=ahe@google.com, karlklose@google.com

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

[karlklose, 2013-11-15 13:39:35]

LGTM.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:46:
print('processClasses ${typeVariableClasses != null}');
Remove debug code.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:57:
// TODO(zarah): Running through all the members is suboptimal. Change this
Reduce indentation by two.

[ngeoffray, 2013-11-18 07:27:51]

DBC

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:68:
!hasMemberNeededForReflection(cls)) return;
Please add braces on a two-line condition.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:104:
evaluator = new CompileTimeConstantEvaluator(
Why is that not created at initialization of this class?

[ahe, 2013-11-18 10:02:45]

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:47:
if (typeVariableClasses != null) {
As discussed, I don't think you can be sure that registerClassWithTypeVariables
is called after tree-shaking has been disabled.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:56:
void processClass(ClassElement cls) {
Could you come up with a more descriptive name?

[zarah, 2013-11-27 14:18:23]

PTAL

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:46:
print('processClasses ${typeVariableClasses != null}');
On 2013/11/15 13:39:35, karlklose wrote:
> Remove debug code.

Done.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:47:
if (typeVariableClasses != null) {
On 2013/11/18 10:02:45, ahe wrote:
> As discussed, I don't think you can be sure that
registerClassWithTypeVariables
> is called after tree-shaking has been disabled.

Changed to use onElementResolved as trigger.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:56:
void processClass(ClassElement cls) {
On 2013/11/18 10:02:45, ahe wrote:
> Could you come up with a more descriptive name?

Done.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:57:
// TODO(zarah): Running through all the members is suboptimal. Change this
On 2013/11/15 13:39:35, karlklose wrote:
> Reduce indentation by two.

Done.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:68:
!hasMemberNeededForReflection(cls)) return;
On 2013/11/18 07:27:52, ngeoffray wrote:
> Please add braces on a two-line condition.

Done.

https://codereview.chromium.org/73403003/diff/40001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:104:
evaluator = new CompileTimeConstantEvaluator(
On 2013/11/18 07:27:52, ngeoffray wrote:
> Why is that not created at initialization of this class?

Since it's not sure it will be needed. Added a getter that initializes it
instead.

[ahe, 2013-11-27 14:45:46]

https://codereview.chromium.org/73403003/diff/110001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/73403003/diff/110001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:44:
compiler.constantHandler, compiler.globalDependencies, compiler);
Why isn't this a field on compiler?

https://codereview.chromium.org/73403003/diff/110001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:101:
if (!enqueuer.isResolutionQueue) return;
Link constructors = typeVariableClass.ensureResolved(compiler).constructors;
if (constructors.isEmpty && constructors.tail.isEmpty) {
  compiler.reportInternalError(
              typeVariableClass,
              "Class '$typeVariableClass' should only have one constructor");
}
typeVariableConstructor = typeVariableClass.constructors.head;
backend.enqueueInResolution(typeVariableConstructor,
compiler.globalDependencies);
enqueuer.registerInstantiatedType(
    typeVariableClass.rawType,
    compiler.globalDependencies);

https://codereview.chromium.org/73403003/diff/110001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:121:
'Class $typeVariableClass should only have one constructor');
Put quotes around class name.

[zarah, 2013-11-27 15:23:17]

PTAL

https://codereview.chromium.org/73403003/diff/110001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/73403003/diff/110001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:44:
compiler.constantHandler, compiler.globalDependencies, compiler);
On 2013/11/27 14:45:46, ahe wrote:
> Why isn't this a field on compiler?
Changed, as discussed.

https://codereview.chromium.org/73403003/diff/110001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:101:
if (!enqueuer.isResolutionQueue) return;
On 2013/11/27 14:45:46, ahe wrote:
> Link constructors = typeVariableClass.ensureResolved(compiler).constructors;
> if (constructors.isEmpty && constructors.tail.isEmpty) {
>   compiler.reportInternalError(
>               typeVariableClass,
>               "Class '$typeVariableClass' should only have one constructor");
> }
> typeVariableConstructor = typeVariableClass.constructors.head;
> backend.enqueueInResolution(typeVariableConstructor,
> compiler.globalDependencies);
> enqueuer.registerInstantiatedType(
>     typeVariableClass.rawType,
>     compiler.globalDependencies);

Done.

https://codereview.chromium.org/73403003/diff/110001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:121:
'Class $typeVariableClass should only have one constructor');
On 2013/11/27 14:45:46, ahe wrote:
> Put quotes around class name.

Done.

[ahe, 2013-11-27 15:43:58]

LGTM!

https://codereview.chromium.org/73403003/diff/130001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/73403003/diff/130001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:100:
void onTreeShakingDisabled(Enqueuer enqueuer) {
Perhaps this should be grouped with registerClassWithTypeVariables above?

[zarah, 2013-11-28 09:24:07]

Committed patchset #3 manually as r30751 (presubmit successful).

[zarah, 2013-11-28 09:29:25]

https://codereview.chromium.org/73403003/diff/130001/sdk/lib/_internal/compil...
File
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart
(right):

https://codereview.chromium.org/73403003/diff/130001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/type_variable_handler.dart:100:
void onTreeShakingDisabled(Enqueuer enqueuer) {
On 2013/11/27 15:43:58, ahe wrote:
> Perhaps this should be grouped with registerClassWithTypeVariables above?

Done.