Generative constructors in the new dart backend.

Generative constructors in the new dart backend.

With super and field initializers.

Field initializers and the arguments to a super-initializer
are treated as zero-arg nested functions, and if they assign
to a parameter, that parameter is set to be a closure
variable.

Still no 'this' initializer redirections.

initializing formals (such as `C(this.x)` ) are translated
to field initializers. We might want to revisit this choice.

R=asgerf@google.com, kmillikin@google.com

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

[sigurdm, 2014-12-08 14:23:11]

Patchset #1 (id:1) has been deleted

[sigurdm, 2014-12-08 14:29:44]

sigurdm@google.com changed reviewers:
+ johnniwinther@google.com, kmillikin@google.com

[floitsch, 2014-12-08 17:45:02]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2014-12-08 17:45:03]

DBC.

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:643: element,
state.functionParameters, makeRunnableBody(), initializers,
assign "makeRunnableBody" to a temporary first, so that we have a name for it.

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:193: if
(element.isGenerativeConstructor) {
Make only one `if`

if (element.isGenerativeConstructor) {
  List<ir.Initializer> initializers = ...;
  visit(node.body);
  return irBuilder.makeConstructorDefinition(defaults, initializers);
} else {
  visit(node.body);
  return irBuilder.makeFunctionDefinition(defaults);
}

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1017: // If a parameter
is stored to in an initializer list we box it.
-to-

[sigurdm, 2014-12-09 09:57:11]

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:643: element,
state.functionParameters, makeRunnableBody(), initializers,
On 2014/12/08 17:45:02, floitsch wrote:
> assign "makeRunnableBody" to a temporary first, so that we have a name for it.

Done.

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:193: if
(element.isGenerativeConstructor) {
On 2014/12/08 17:45:02, floitsch wrote:
> Make only one `if`
> 
> if (element.isGenerativeConstructor) {
>   List<ir.Initializer> initializers = ...;
>   visit(node.body);
>   return irBuilder.makeConstructorDefinition(defaults, initializers);
> } else {
>   visit(node.body);
>   return irBuilder.makeFunctionDefinition(defaults);
> }

Done.

https://codereview.chromium.org/787603003/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1017: // If a parameter
is stored to in an initializer list we box it.
On 2014/12/08 17:45:03, floitsch wrote:
> -to-

Done.

[asgerf, 2014-12-10 07:56:32]

asgerf@google.com changed reviewers:
+ asgerf@google.com

[asgerf, 2014-12-10 07:56:33]

LGTM. But the diff seems to contain stuff from other CLs so I can easily have
overlooked something. Could you try rebasing?

[sigurdm, 2014-12-10 09:04:02]

I tried rebase. Hope it looks better

[asgerf, 2014-12-10 10:23:55]

Still LGTM.

[Kevin Millikin (Google), 2014-12-11 09:28:25]

A few simple comments.  LGTM when they are addressed.

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:583: ir.SuperInitializer
makeSuperInitializer(ConstructorElement target,
Is there any reason that this function takes the arguments in a different order
than the constructor?

If one order is better than the other, it seems like they should both use it.

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:652: state.localConstants,
defaults, element.isConst,
Since we're passing in element, maybe it's better to just take isConst from the
element instead of requiring passing it.  Or you could make
ConstructorDefinition.isConst a getter that uses element.isConst.

Or do you think we will ever want to pass a value to the constructor that
disagrees with element.isConst?

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:205:
List<ir.Initializer> result = new List<ir.Initializer>();
<ir.Initializer>[];

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:265: new
Selector.callDefaultConstructor(enclosingClass.library);
Too much indentation here.

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1020:
markAsClosureVariable(local);
For now this is OK, but I think later we will want to distinguish between
variables that are really captured by closures and those that just need to be
boxed for the execution of the function (like these) but don't outlive it.

Could you add a comment that says something like: TODO: Fix this.  Though these
variables do not outlive the activation of the function, they still need to be
boxed.  As a simplification, we treat them as if they are captured by a closure
(i.e., they do outlive the activation of the function).

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1022:
node.visitChildren(this);
This function visits all the children twice (once here and once in visitSend). 

I'm pretty sure we want to avoid that for all kinds of reasons.

I'm not sure what's the best way to handle it.  Instead of calling
visitSend+visitChildren here, you could just call super (which does exactly
that).  Or just don't call visitChildren explicitly here.

[sigurdm, 2014-12-17 10:20:56]

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:583: ir.SuperInitializer
makeSuperInitializer(ConstructorElement target,
On 2014/12/11 09:28:25, kmillikin wrote:
> Is there any reason that this function takes the arguments in a different
order
> than the constructor?
> 
> If one order is better than the other, it seems like they should both use it.

No reason besides oversight.

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:652: state.localConstants,
defaults, element.isConst,
On 2014/12/11 09:28:25, kmillikin wrote:
> Since we're passing in element, maybe it's better to just take isConst from
the
> element instead of requiring passing it.  Or you could make
> ConstructorDefinition.isConst a getter that uses element.isConst.
> 
> Or do you think we will ever want to pass a value to the constructor that
> disagrees with element.isConst?

Removed the isConst property from tre_ir and cps_ir, just reading it from the
element when it is needed.

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:205:
List<ir.Initializer> result = new List<ir.Initializer>();
On 2014/12/11 09:28:25, kmillikin wrote:
> <ir.Initializer>[];

Done.

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:265: new
Selector.callDefaultConstructor(enclosingClass.library);
On 2014/12/11 09:28:25, kmillikin wrote:
> Too much indentation here.

Done.

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1020:
markAsClosureVariable(local);
On 2014/12/11 09:28:25, kmillikin wrote:
> For now this is OK, but I think later we will want to distinguish between
> variables that are really captured by closures and those that just need to be
> boxed for the execution of the function (like these) but don't outlive it.
> 
> Could you add a comment that says something like: TODO: Fix this.  Though
these
> variables do not outlive the activation of the function, they still need to be
> boxed.  As a simplification, we treat them as if they are captured by a
closure
> (i.e., they do outlive the activation of the function).

Acknowledged.

https://codereview.chromium.org/787603003/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1022:
node.visitChildren(this);
On 2014/12/11 09:28:25, kmillikin wrote:
> This function visits all the children twice (once here and once in visitSend).

> 
> I'm pretty sure we want to avoid that for all kinds of reasons.
> 
> I'm not sure what's the best way to handle it.  Instead of calling
> visitSend+visitChildren here, you could just call super (which does exactly
> that).  Or just don't call visitChildren explicitly here.

Made a helper that both visitSend and visitSendSet can call.

[sigurdm, 2014-12-17 10:34:41]

Committed patchset #5 (id:100001) manually as 42427 (presubmit successful).