Implement constructor bodies and initializers in CPS->JS backend.

Implement constructor bodies and initializers in CPS->JS backend.

As in the SSA backend, a constructor consists of three parts:
1. Instantiator: creates an object and sets its fields
2. Factory: evaluates field initializers, calls instantiator, then
   calls bodies.
3. Body: executes the body of a constructor.

The CreateClosureClass instruction has been replaced by CreateInstance
for invoking the instantiator for an arbitrary class.

The IrBuilderVisitor has been split into two subclasses to better
isolate the JS-specific code.

BUG=
R=floitsch@google.com

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

[asgerf, 2015-01-20 16:13:16]

asgerf@google.com changed reviewers:
+ floitsch@google.com, kmillikin@google.com

[asgerf, 2015-01-20 16:13:17]

https://codereview.chromium.org/862703002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (left):

https://codereview.chromium.org/862703002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:162: }
All this stuff have been moved to subclasses.

https://codereview.chromium.org/862703002/diff/1/pkg/compiler/lib/src/cps_ir/...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/862703002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1013: class
DartIrBuilderVisitor extends IrBuilderVisitor {
This is mostly existing stuff that has been moved here.

https://codereview.chromium.org/862703002/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1231: 
This is where the constructor-building code beings.

[floitsch, 2015-01-20 17:26:27]

First round of comments.
Will have another look, once we agree on the initialization order.

https://codereview.chromium.org/862703002/diff/20001/pkg/analyzer2dart/lib/sr...
File pkg/analyzer2dart/lib/src/cps_generator.dart (right):

https://codereview.chromium.org/862703002/diff/20001/pkg/analyzer2dart/lib/sr...
pkg/analyzer2dart/lib/src/cps_generator.dart:53: DartIrBuilder get irBuilder =>
super.irBuilder;
Explain why.

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

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:350: {ClosureScope closureScope,
indentation

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1977: ClosureScope closureScope)
{
indentation

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

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1032: /// Returns the
backend-specific representation of an inner function.
That comment can be updated.

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1071: ast.Node
fieldDefinition =
should fit on one line, no?

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1132: /// Returns the
backend-specific representation of an inner function.
Update comment.

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1158: // Converts
closure.dart's CapturedVariable into a ClosureLocation.
Make a dartdoc.

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1232: /// During
construction of a constructor factory, [fieldValues] maps fields
Move it up to the locals.

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1234: Map<FieldElement,
ir.Primitive> fieldValues = {};
type the map.

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1263: // Evaluate all
initializers on from field declarations.
-on-

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1264: // For some
reason, these should ALL be evaluated first, even before
I'm pretty sure this is not true.

I tried it with a small example and it ran through the initializers in order.

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1440: * Returns [:null:]
if the constructor does not have a body.
`null`

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:220: // Constructor body
cannot be overriden or intercepted, so we can
For this to work, we need to guarantee that the closure names are unique. 

A short test seems to indicate that this is not the case:

$ cat a.dart
library a;
import 'b.dart' as b;

class A extends b.A {
  A() {
    print("a");
    try {} catch(e) {} // no inline.
  }
}


main() {
  new A();
}

$ cat b.dart
library b;

class A {
  A() {
    print("Ab");
    try {} catch(e) {} // no inline.
  }
}


from out.js:
["a", "a.dart", , O, {
  "^": "",
  main: function() {
    O.A$();
  },
  A: {
    "^": "A0;",
    A$0: function() {
      var exception;
...
["b", "b.dart", , E, {
  "^": "",
  A0: {
    "^": "Object;",
    A$0: function() {
      var exception;
      P.print("Ab");
...

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/glue.dart (right):

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/glue.dart:89: return
_backend.emitter.internalConstructorAccess(element);
There is already a "constructorAccess". Is that one not good enough?

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_emitter/new_emitter/emitter.dart (right):

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_emitter/new_emitter/emitter.dart:144: return
_globalPropertyAccess(element);
I missed this in my last review.
This won't work.

As written in glue.dart I would expect this method to go away.

[asgerf, 2015-01-21 09:55:58]

Done all the minor fixes.

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

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1264: // For some
reason, these should ALL be evaluated first, even before
On 2015/01/20 17:26:27, floitsch wrote:
> I'm pretty sure this is not true.
> 
> I tried it with a small example and it ran through the initializers in order.

You are right. The SSA builder had me confused, and somehow I must have messed
up my own test case when I tried it.

Will fix this after the minor fixes.

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:220: // Constructor body
cannot be overriden or intercepted, so we can
On 2015/01/20 17:26:27, floitsch wrote:
> For this to work, we need to guarantee that the closure names are unique. 
> 
> A short test seems to indicate that this is not the case:
> 
> $ cat a.dart
> library a;
> import 'b.dart' as b;
> 
> class A extends b.A {
>   A() {
>     print("a");
>     try {} catch(e) {} // no inline.
>   }
> }
> 
> 
> main() {
>   new A();
> }
> 
> $ cat b.dart
> library b;
> 
> class A {
>   A() {
>     print("Ab");
>     try {} catch(e) {} // no inline.
>   }
> }
> 
> 
> from out.js:
> ["a", "a.dart", , O, {
>   "^": "",
>   main: function() {
>     O.A$();
>   },
>   A: {
>     "^": "A0;",
>     A$0: function() {
>       var exception;
> ...
> ["b", "b.dart", , E, {
>   "^": "",
>   A0: {
>     "^": "Object;",
>     A$0: function() {
>       var exception;
>       P.print("Ab");
> ...
> 

Nice catch.

Since the bug also exists in the sable version of dart2js, should we fix it in a
separate CL? I think the namer is the place to fix it.

[floitsch, 2015-01-21 11:37:54]

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/862703002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:220: // Constructor body
cannot be overriden or intercepted, so we can
On 2015/01/21 09:55:58, asgerf wrote:
> On 2015/01/20 17:26:27, floitsch wrote:
> > For this to work, we need to guarantee that the closure names are unique. 
> > 
> > A short test seems to indicate that this is not the case:
> > 
> > $ cat a.dart
> > library a;
> > import 'b.dart' as b;
> > 
> > class A extends b.A {
> >   A() {
> >     print("a");
> >     try {} catch(e) {} // no inline.
> >   }
> > }
> > 
> > 
> > main() {
> >   new A();
> > }
> > 
> > $ cat b.dart
> > library b;
> > 
> > class A {
> >   A() {
> >     print("Ab");
> >     try {} catch(e) {} // no inline.
> >   }
> > }
> > 
> > 
> > from out.js:
> > ["a", "a.dart", , O, {
> >   "^": "",
> >   main: function() {
> >     O.A$();
> >   },
> >   A: {
> >     "^": "A0;",
> >     A$0: function() {
> >       var exception;
> > ...
> > ["b", "b.dart", , E, {
> >   "^": "",
> >   A0: {
> >     "^": "Object;",
> >     A$0: function() {
> >       var exception;
> >       P.print("Ab");
> > ...
> > 
> 
> Nice catch.
> 
> Since the bug also exists in the sable version of dart2js, should we fix it in
a
> separate CL? I think the namer is the place to fix it.

maybe. I actually thought that we had direct calls in the current version.
(Didn't even try to run it).

[asgerf, 2015-01-21 12:48:26]

PTAL

Initialization order fixed.

Also found and fixed a bug when inlining expressions from field initializers.

Sorry for the messy patch history btw.

[asgerf, 2015-01-21 15:44:14]

Patchset #8 (id:140001) has been deleted

[asgerf, 2015-01-21 15:50:51]

Patchset #8 (id:160001) has been deleted

[floitsch, 2015-01-21 16:29:35]

(no worries for the patch history).

LGTM.
nice clean code.

The naming for the closure things is not perfect (I guess we can blame me on
that), but we can try to improve it later.

https://codereview.chromium.org/862703002/diff/120001/pkg/analyzer2dart/lib/s...
File pkg/analyzer2dart/lib/src/cps_generator.dart (right):

https://codereview.chromium.org/862703002/diff/120001/pkg/analyzer2dart/lib/s...
pkg/analyzer2dart/lib/src/cps_generator.dart:54: /// The JS backend requires
closure conversion which we do not support yet.
new line before?

Just looking at this comment it is not clear what closure conversion has to do
with the up cast.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1024:
DetectClosureVariables closures = new DetectClosureVariables(elements);
This does not sound like a class name.
Is this a ClosureVariableDetector?

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1124: Compiler compiler,
indentation.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1129: closureMap =
compiler.closureToClassMapper.computeClosureToClassMapping(
This requires comments, and potentially a refactoring.

If I understand correctly `makeIRBuilder` is only called from the super-class,
when the irBuilder is set.
However, the way it's currently done is brittle.
What about making "withFreshBuilder" an abstract class? Then irBuilder should be
set by the sub class, and it makes sense to set an independent local.

In short: setting the closureMap variable has nothing to do with creating a new
builder, and seems to happen under the assumption that the new builder is used
immediately. We should not just depend on the assumption.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1157:
map.forEach((key,value) {
space after ','

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1176: /// captured
reference to `this`), this returns a [ClosureEnvironment]
-this-

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1186: /// If [node] has
declarations for variables that should be boxed, this
-this-

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1192:
closurelib.ClosureScope scope = closureMap.capturingScopes[node];
This is weird: there is an import without the prefix.
It feels like ClosureScope should at least give you a warning (but maybe
shadowing from within a library doesn't need one).

If the import without the prefix stays (which I'm ok with), it should have a
"hide ClosureScope" clause.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1204: ClosureScope
getFunctionScope(FunctionElement function) {
Naming feels weird.

I would have a tendency to call the nested functions "closure" and the outer one
less.

maybe?
getClosureScopeForNode
getClosureScopeForFunction

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1249: /// Builds the IR
for a given constructor. The performs the following tasks:
Remove the part after the "."

(It's pretty clear that the enumerated list is what it does).

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1251: /// 1. Evaluate
all own or inherited field initializers.
Evaluates

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1252: /// 2. Create the
object and assign its fields.
Creates  ... assigns

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1253: /// 3. Call
constructor body and super constructor bodies.
Calls

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1254: /// 4. Return the
created object.
Returns

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1286: // Native fields
are initialized elsewhere.
can we assert that the field comes from a native class?
assert(Elements.isNativeOrExtendsNative(field.enclosingElement)) ?

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1296: if (bodyElement ==
null) continue;
Comment when this can happen.

I'm guessing when the constructor does not have any body?
(E.g. `A(this.x);`)

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1320: /// The parameters
to superconstructors are, however, assumed NOT to be in
*not*

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1408: ++index;
tiny nit: we generally use index++.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1432: ++index;
ditto.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1467: List<Local>
parameters = getConstructorBodyParameters(constructor);
leftover?

getConstructorBodyParameters shouldn't have a side-effect. (If it has, we should
change the name).

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1486: /// to the
generative constructor body.
Maybe add note that the returned list may contain boxes, and has the boxed
variables removed.

Ideally give an example of why/what we are doing.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1516: JsIrBuilder
builder = new JsIrBuilder(
nit:
I prefer not to break expressions:

JsIrBuilder builder =
    new JsIrBuilder(...);

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:220: // Constructor body
cannot be overriden or intercepted, so we can
A constructor ...

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:221: // use the short form
for this invocation.
or not ;)

[asgerf, 2015-01-22 10:18:58]

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1024:
DetectClosureVariables closures = new DetectClosureVariables(elements);
On 2015/01/21 16:29:34, floitsch wrote:
> This does not sound like a class name.
> Is this a ClosureVariableDetector?

I don't really buy into the "classes are nouns, methods are verbs" way of naming
things, but this does seems to be the trend here.

This is just code that got moved here because of the separation into Dart- and
JS-specific visitors, but I'll just do the rename here so we're done with it.

I've changed it to DartCapturedVariables to indicate that it is specific to the
Dart backend, and got rid of the misleading use of the word "closure".

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1124: Compiler compiler,
On 2015/01/21 16:29:35, floitsch wrote:
> indentation.

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1129: closureMap =
compiler.closureToClassMapper.computeClosureToClassMapping(
On 2015/01/21 16:29:35, floitsch wrote:
> This requires comments, and potentially a refactoring.
> 
> If I understand correctly `makeIRBuilder` is only called from the super-class,
> when the irBuilder is set.
> However, the way it's currently done is brittle.
> What about making "withFreshBuilder" an abstract class? Then irBuilder should
be
> set by the sub class, and it makes sense to set an independent local.
> 
> In short: setting the closureMap variable has nothing to do with creating a
new
> builder, and seems to happen under the assumption that the new builder is used
> immediately. We should not just depend on the assumption.

It's a leftover from before the separate subclasses were made. closureMap is
supposed to be initialized before entering a body of code.

The method is only called once; from the subclass. It does not implement any
abstract method. I've inlined this one to clarify things a bit.

I understand the initial confusion, but it should be better now, and I don't
think there is need for more scaffolding code to manage this one field.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1157:
map.forEach((key,value) {
On 2015/01/21 16:29:34, floitsch wrote:
> space after ','

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1176: /// captured
reference to `this`), this returns a [ClosureEnvironment]
On 2015/01/21 16:29:34, floitsch wrote:
> -this-

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1186: /// If [node] has
declarations for variables that should be boxed, this
On 2015/01/21 16:29:35, floitsch wrote:
> -this-

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1192:
closurelib.ClosureScope scope = closureMap.capturingScopes[node];
On 2015/01/21 16:29:34, floitsch wrote:
> This is weird: there is an import without the prefix.
> It feels like ClosureScope should at least give you a warning (but maybe
> shadowing from within a library doesn't need one).
> 
> If the import without the prefix stays (which I'm ok with), it should have a
> "hide ClosureScope" clause.

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1204: ClosureScope
getFunctionScope(FunctionElement function) {
On 2015/01/21 16:29:35, floitsch wrote:
> Naming feels weird.
> 
> I would have a tendency to call the nested functions "closure" and the outer
one
> less.
> 
> maybe?
> getClosureScopeForNode
> getClosureScopeForFunction

Yes the naming wasn't that great. Changed to the names you suggested.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1249: /// Builds the IR
for a given constructor. The performs the following tasks:
On 2015/01/21 16:29:35, floitsch wrote:
> Remove the part after the "."
> 
> (It's pretty clear that the enumerated list is what it does).

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1251: /// 1. Evaluate
all own or inherited field initializers.
On 2015/01/21 16:29:35, floitsch wrote:
> Evaluates

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1252: /// 2. Create the
object and assign its fields.
On 2015/01/21 16:29:34, floitsch wrote:
> Creates  ... assigns

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1253: /// 3. Call
constructor body and super constructor bodies.
On 2015/01/21 16:29:35, floitsch wrote:
> Calls

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1254: /// 4. Return the
created object.
On 2015/01/21 16:29:35, floitsch wrote:
> Returns

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1286: // Native fields
are initialized elsewhere.
On 2015/01/21 16:29:34, floitsch wrote:
> can we assert that the field comes from a native class?
> assert(Elements.isNativeOrExtendsNative(field.enclosingElement)) ?

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1296: if (bodyElement ==
null) continue;
On 2015/01/21 16:29:34, floitsch wrote:
> Comment when this can happen.
> 
> I'm guessing when the constructor does not have any body?
> (E.g. `A(this.x);`)

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1320: /// The parameters
to superconstructors are, however, assumed NOT to be in
On 2015/01/21 16:29:35, floitsch wrote:
> *not*

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1408: ++index;
On 2015/01/21 16:29:34, floitsch wrote:
> tiny nit: we generally use index++.

I prefer that too, but have been told the exact opposite in another review.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1432: ++index;
On 2015/01/21 16:29:34, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1467: List<Local>
parameters = getConstructorBodyParameters(constructor);
On 2015/01/21 16:29:34, floitsch wrote:
> leftover?
> 
> getConstructorBodyParameters shouldn't have a side-effect. (If it has, we
should
> change the name).

You are right, it was a leftover (I previously stored the parameter list of the
element, created below, but then decided against it).

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1486: /// to the
generative constructor body.
On 2015/01/21 16:29:34, floitsch wrote:
> Maybe add note that the returned list may contain boxes, and has the boxed
> variables removed.
> 
> Ideally give an example of why/what we are doing.

Done.

I also changed the parameter to ConstructorBodyElement to clarify that the
body's element should be given, not the generative constructor.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1516: JsIrBuilder
builder = new JsIrBuilder(
On 2015/01/21 16:29:35, floitsch wrote:
> nit:
> I prefer not to break expressions:
> 
> JsIrBuilder builder =
>     new JsIrBuilder(...);

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/codegen.dart (right):

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:220: // Constructor body
cannot be overriden or intercepted, so we can
On 2015/01/21 16:29:35, floitsch wrote:
> A constructor ...

Done.

https://codereview.chromium.org/862703002/diff/120001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:221: // use the short form
for this invocation.
On 2015/01/21 16:29:35, floitsch wrote:
> or not ;)

Inserted a TODO. Will update after the fix lands.

[floitsch, 2015-01-22 10:36:01]

Still LGTM.

https://codereview.chromium.org/862703002/diff/220001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/862703002/diff/220001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1117: ClosureClassMap
closureMap;
Add a small comment what this is and where it comes from (also where it is set).

[asgerf, 2015-01-22 10:58:29]

Thanks for the very thorough reviews!

https://codereview.chromium.org/862703002/diff/220001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart (right):

https://codereview.chromium.org/862703002/diff/220001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1117: ClosureClassMap
closureMap;
On 2015/01/22 10:36:01, floitsch wrote:
> Add a small comment what this is and where it comes from (also where it is
set).

Done.

[asgerf, 2015-01-23 10:19:46]

Committed patchset #13 (id:280001) manually as 43092 (presubmit successful).