Implement super initializers.

Implement super initializers.

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

[ngeoffray, 2012-02-07 14:25:59]

DBC

http://codereview.chromium.org/9327001/diff/4001/frog/leg/resolver.dart
File frog/leg/resolver.dart (right):

http://codereview.chromium.org/9327001/diff/4001/frog/leg/resolver.dart#newco...
frog/leg/resolver.dart:44: case ElementKind.GENERATIVE_CONSTRUCTOR_BODY:
Why are you adding this one? The generative constructor body should only go
through codegen.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:455: if (body.constructor== constructor) {
space missing before '=='

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:474: * Call [forEach] for every argument and parameter
and element of [target]
Shouldn't it just be 'argument'? If there are remaining parameters you're not
doing anything with them.

Also, there is an addStaticSendArgumentsToList already in that file. Maybe you
could use this method instead, because it looks like it's the same functionality
that is needed (matching a statically resolved call to the method definition).

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:506: final Map<FunctionElement, TreeElements>
constructorElements =
Why is this one final and not the others? For consistency, I would not put final
on local variables.

Also, this variable is unused.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:510: new Map<FunctionElement, List<HInstruction>>();
If this method was a LinkedHashMap, would you need the constructors list?

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:513: FunctionElement next = functionElement;
next -> nextSuperCall ?

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:517: });
strange indentation.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:531: if (initializers === null) continue;
break?

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:534: // analyze referenced superinitializers and
constructor redirections.
Maybe add a function for the code running through the initializers, that return
the nextSuperCall ?

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:545: FunctionExpression superNode =
superconstructor.parseNode(compiler);
Unused variable.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:548:
superconstructor.functionParameters.requiredParameters;
This variable seems unnecessary.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:592: constructorArguments.add(value);
strange indentation.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:606: for (HInstruction value in environment) {
Instead of creating a new list and adding the instructions, you could reuse the
same list, and make sure you have an empty slot at the beginning.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:615: add(new HInvokeDynamicMethod(selector,
methodName, bodyCallInputs));
I believe you don't need a selector for this HInvokeDynamic, why did you add
one?

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:1475: HInstruction updateElementDefinition(Element
element, HInstruction value) {
Since this method is only used once, I would inline it.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/9327001/diff/4001/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:1466: }
extra space.

[floitsch, 2012-02-07 17:19:37]

FYI.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:136: HGraph compileConstructorBody(SsaBuilder builder,
remove this method.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:142: HGraph compileMethod(SsaBuilder builder,
ditto.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:463: compiler.enqueue(
one line?

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:468: assert(bodyElement !== null &&
bodyElement is trivially non null.
I would remove the first part of the assert. But it's not hurting anybody.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:478: forEach(VariableElement parameter, Node
argument)) {
please name to something else. Even 'f' would be better imho.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:493: * Build the factory function corresponding to the
constructor [bodyElement]:
s/bodyElement/functionElement.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:527: compiler.enqueue(new
WorkItem.toCompile(constructor));
remove enqueuing here.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:531: if (initializers === null) continue;
We have to find the implicit super-constructor instead.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:535: for (Link<Node> link = initializers.nodes;
once you evaluate the initializers with the current localsHandler you could move
this code into a function.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:542: // A super initializer or constructor
redirection.
move comment one line up.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:543: if
(Initializers.isSuperConstructorCall(link.head)) {
s/link.head/call

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:544: FunctionElement superconstructor =
elements[link.head];
superConstructor and s/link.head/call.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:582: while (element != null) {
add TODO to share this code with the emitter. There should be a function on the
ClassElement.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:589: value = new HLiteral(null, HType.UNKNOWN);
Add TODO that we need to get the default-values if there are some.

[karlklose, 2012-02-08 14:12:00]

Thanks for the comments, Nicolas and Florian. PTAL.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:136: HGraph compileConstructorBody(SsaBuilder builder,
On 2012/02/07 17:19:37, floitsch wrote:
> remove this method.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:142: HGraph compileMethod(SsaBuilder builder,
On 2012/02/07 17:19:38, floitsch wrote:
> ditto.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:455: if (body.constructor== constructor) {
On 2012/02/07 14:25:59, ngeoffray wrote:
> space missing before '=='

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:463: compiler.enqueue(
On 2012/02/07 17:19:38, floitsch wrote:
> one line?

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:468: assert(bodyElement !== null &&
On 2012/02/07 17:19:38, floitsch wrote:
> bodyElement is trivially non null.
> I would remove the first part of the assert. But it's not hurting anybody.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:474: * Call [forEach] for every argument and parameter
and element of [target]
On 2012/02/07 14:25:59, ngeoffray wrote:
> Shouldn't it just be 'argument'? If there are remaining parameters you're not
> doing anything with them.
> 
> Also, there is an addStaticSendArgumentsToList already in that file. Maybe you
> could use this method instead, because it looks like it's the same
functionality
> that is needed (matching a statically resolved call to the method definition).

Done. I do not use environments anymore, so addStatucSendArgumentsToList is not
usable directly.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:478: forEach(VariableElement parameter, Node
argument)) {
On 2012/02/07 17:19:38, floitsch wrote:
> please name to something else. Even 'f' would be better imho.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:493: * Build the factory function corresponding to the
constructor [bodyElement]:
On 2012/02/07 17:19:38, floitsch wrote:
> s/bodyElement/functionElement.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:506: final Map<FunctionElement, TreeElements>
constructorElements =
Done, I removed unused variables and the environments.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:510: new Map<FunctionElement, List<HInstruction>>();
see above.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:513: FunctionElement next = functionElement;
Changed to nextConstructor.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:517: });
Removed.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:527: compiler.enqueue(new
WorkItem.toCompile(constructor));
On 2012/02/07 17:19:38, floitsch wrote:
> remove enqueuing here.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:531: if (initializers === null) continue;
On 2012/02/07 17:19:38, floitsch wrote:
> We have to find the implicit super-constructor instead.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:531: if (initializers === null) continue;
Changed to set initializers to an empty list.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:534: // analyze referenced superinitializers and
constructor redirections.
On 2012/02/07 14:25:59, ngeoffray wrote:
> Maybe add a function for the code running through the initializers, that
return
> the nextSuperCall ?

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:535: for (Link<Node> link = initializers.nodes;
On 2012/02/07 17:19:38, floitsch wrote:
> once you evaluate the initializers with the current localsHandler you could
move
> this code into a function.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:542: // A super initializer or constructor
redirection.
On 2012/02/07 17:19:38, floitsch wrote:
> move comment one line up.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:543: if
(Initializers.isSuperConstructorCall(link.head)) {
On 2012/02/07 17:19:38, floitsch wrote:
> s/link.head/call

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:544: FunctionElement superconstructor =
elements[link.head];
On 2012/02/07 17:19:38, floitsch wrote:
> superConstructor and s/link.head/call.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:545: FunctionExpression superNode =
superconstructor.parseNode(compiler);
On 2012/02/07 14:25:59, ngeoffray wrote:
> Unused variable.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:548:
superconstructor.functionParameters.requiredParameters;
On 2012/02/07 14:25:59, ngeoffray wrote:
> This variable seems unnecessary.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:582: while (element != null) {
On 2012/02/07 17:19:38, floitsch wrote:
> add TODO to share this code with the emitter. There should be a function on
the
> ClassElement.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:589: value = new HLiteral(null, HType.UNKNOWN);
On 2012/02/07 17:19:38, floitsch wrote:
> Add TODO that we need to get the default-values if there are some.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:592: constructorArguments.add(value);
On 2012/02/07 14:25:59, ngeoffray wrote:
> strange indentation.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:606: for (HInstruction value in environment) {
I removed the environments.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:615: add(new HInvokeDynamicMethod(selector,
methodName, bodyCallInputs));
Removed.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:1475: HInstruction updateElementDefinition(Element
element, HInstruction value) {
On 2012/02/07 14:25:59, ngeoffray wrote:
> Since this method is only used once, I would inline it.

Done.

http://codereview.chromium.org/9327001/diff/4001/frog/leg/tree/nodes.dart
File frog/leg/tree/nodes.dart (right):

http://codereview.chromium.org/9327001/diff/4001/frog/leg/tree/nodes.dart#new...
frog/leg/tree/nodes.dart:1466: }
On 2012/02/07 14:25:59, ngeoffray wrote:
> extra space.

Done.

[floitsch, 2012-02-08 15:30:27]

LGTM.

http://codereview.chromium.org/9327001/diff/9001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9327001/diff/9001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:569: if (enclosingClass.name != Types.OBJECT) {
use:  compiler.coreLibrary.find(const SourceString('Object'));

[karlklose, 2012-02-08 17:20:44]

Thanks for the review.

https://chromiumcodereview.appspot.com/9327001/diff/9001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9327001/diff/9001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:569: if (enclosingClass.name != Types.OBJECT) {
On 2012/02/08 15:30:27, floitsch wrote:
> use:  compiler.coreLibrary.find(const SourceString('Object'));

Done.

[ngeoffray, 2012-02-09 08:25:50]

LGTM!

http://codereview.chromium.org/9327001/diff/9001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9327001/diff/9001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:548: compiler.resolver.constructorElements;
This variable is unused.

http://codereview.chromium.org/9327001/diff/9001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:612:
body.functionParameters.forEachParameter((parameter) {
To be safe, should that be body.computeParameters(compiler).forEachParameter ?

[ngeoffray, 2012-02-09 08:27:02]

Also,

http://codereview.chromium.org/9327001/diff/9001/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

http://codereview.chromium.org/9327001/diff/9001/frog/leg/ssa/builder.dart#ne...
frog/leg/ssa/builder.dart:581: }
I would put 'elements' to null here, to make sure no one tries to use it from
now on.