Add type arguments to constants.

Add type arguments to constants.

R=ngeoffray@google.com

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

[ngeoffray, 2013-06-17 09:15:03]

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:83: if
(setRuntimeTypeInfoFunction != null) return;
In non-checked mode, how do you make sure this helper is not emitted if not
needed? It looks like you're unconditionally adding it, but don't you need to
check if there are is checks that require it?

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:358:
handler.registerSetRuntimeTypeInfoFunction();
Move this check to [registerInstantiatedType]?

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:408: if
(!type.isRaw) {
Ditto.

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:669:
FunctionElement constructor = functionElement;
What is this double assignment for? If line 678 changes the constructor, it
should not matter right?

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:329:
} else {
Same comment as in compile_time_constants: you don't need to do this if there is
no is checks that require it.

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

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3643:
compiler.enqueuer.codegen.registerInstantiatedType(constructed.type,
Isn't this already done by the constant_handler?

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/lib/js_r...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/lib/js_r...
sdk/lib/_internal/lib/js_rti.dart:11: return target;
Why this change?

[karlklose, 2013-06-19 15:29:05]

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compile_time_constants.dart
(right):

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:83: if
(setRuntimeTypeInfoFunction != null) return;
Added a check.

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:358:
handler.registerSetRuntimeTypeInfoFunction();
On 2013/06/17 09:15:03, ngeoffray wrote:
> Move this check to [registerInstantiatedType]?

Done.

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:408: if
(!type.isRaw) {
On 2013/06/17 09:15:03, ngeoffray wrote:
> Ditto.

Done.

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:669:
FunctionElement constructor = functionElement;
Yes, this is left-over code. Removed.

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:329:
} else {
On 2013/06/17 09:15:03, ngeoffray wrote:
> Same comment as in compile_time_constants: you don't need to do this if there
is
> no is checks that require it.

Done.

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/lib/js_r...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/16549004/diff/7001/sdk/lib/_internal/lib/js_r...
sdk/lib/_internal/lib/js_rti.dart:11: return target;
I use the pattern
  $.constant = setRuntimeTypeInfo(new Constant(a, b, ...), [$.JSInt])
to set the runtime type information.

[ngeoffray, 2013-06-19 19:55:48]

LGTM!

https://codereview.chromium.org/16549004/diff/19001/pkg/pkg.status
File pkg/pkg.status (left):

https://codereview.chromium.org/16549004/diff/19001/pkg/pkg.status#oldcode103
pkg/pkg.status:103: pathos/test/pathos_dartium_test: Crash
Is this related?

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

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:64: if
(type is InterfaceType && !type.isRaw &&
nit: move !type.isRaw to a new line.

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:667:
FunctionElement constructor = functionElement;
Didn't you say you removed this?

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

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:327:
if (type is InterfaceType && !type.isRaw &&
nit: move && !type.isRaw to a new line.

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_rti.dart:71: return target;
Please add a comment after line 65 on what this method returns and why.

[karlklose, 2013-06-20 09:25:23]

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

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:327:
if (type is InterfaceType && !type.isRaw &&
Why?

[ngeoffray, 2013-06-20 09:31:55]

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

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:327:
if (type is InterfaceType && !type.isRaw &&
On 2013/06/20 09:25:23, karlklose wrote:
> Why?

http://www.dartlang.org/articles/style-guide/: DO place the operator on the
preceding line in a multi-line expression.

And I think Java follows the same.

Also, I think it's how we format it in our code base, so for consistency.

[karlklose, 2013-06-20 10:23:28]

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

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:327:
if (type is InterfaceType && !type.isRaw &&
But your comment was about moving every part expression on its own line, wasn't
it? Also I do place the operator at the end of the line before the break.

As for the code base and the position of the operator: we have both styles in
the code base but I prefer placing it at the end of the line.

[ngeoffray, 2013-06-20 10:32:07]

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

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:327:
if (type is InterfaceType && !type.isRaw &&
On 2013/06/20 10:23:28, karlklose wrote:
> But your comment was about moving every part expression on its own line,
wasn't
> it? Also I do place the operator at the end of the line before the break.

The guide does not explicitly say it, but if you look at the example, it's
breaking at every part. That's what I think Java style does.
 
> 
> As for the code base and the position of the operator: we have both styles in
> the code base but I prefer placing it at the end of the line.

That's fine, I wasn't talking about the place of the operator.

[karlklose, 2013-06-20 12:49:20]

https://codereview.chromium.org/16549004/diff/19001/pkg/pkg.status
File pkg/pkg.status (left):

https://codereview.chromium.org/16549004/diff/19001/pkg/pkg.status#oldcode103
pkg/pkg.status:103: pathos/test/pathos_dartium_test: Crash
No, I rebased on a crashing version.

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

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:64: if
(type is InterfaceType && !type.isRaw &&
On 2013/06/19 19:55:48, ngeoffray wrote:
> nit: move !type.isRaw to a new line.

Done.

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compile_time_constants.dart:667:
FunctionElement constructor = functionElement;
It was needed to preserve the original constructor to calculate the targetType
substitution below. I moved the type calculation up to avoid the double
assignment.

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

https://codereview.chromium.org/16549004/diff/19001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:327:
if (type is InterfaceType && !type.isRaw &&
Done.

[karlklose, 2013-06-20 12:52:26]

Committed patchset #8 manually as r24223 (presubmit successful).