Use closure conversion in new dart2js backend.

Use closure conversion in new dart2js backend.

New instructions: CreateBox, SetField, GetField, CreateClosureClass.

The "old" instructions based on ref-cells are still in use by dart2dart:
GetClosureVariable, SetClosureVariable, CreateFunction.

dart2dart still does not support captured for-loop variables, but this
is a separate issue.

BUG=
R=floitsch@google.com, kmillikin@google.com, sigurdm@google.com

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

[asgerf, 2015-01-08 14:38:12]

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

[floitsch, 2015-01-08 18:29:37]

LGTM! with some comments.
mostly more documentation, so nothing big.

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

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:183: class IrBuilderClosureState
{
Probably different CL, but I'm not sure the names are really good:
the state that is active for all nested functions is the "ClosureState", but the
state that is active for just my local function (aka "closure") is called
"SharedState".

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:228: /// If non-null, this
refers to the receiver in the enclosing method.
to the receiver (`this`) in ...

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:241: void
declareLocalVariable(LocalVariableElement element, {ir.Primitive initialValue});
long line.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:247: void
_enterClosureEnvironment(ClosureEnvironment env);
comments!

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:259: IrBuilderClosureState
closure;
Again, probably different CL:
closureState, or even sharedState (if we rename the classes).

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:262: /// [BoxLocal]s map to
their box. [LocalElement]s that are boxed are not
New line before. (Otherwise markdown merges them).

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:293: void _init(ConstantSystem
constantSystem, ExecutableElement currentElement) {
add comment.

Could you keep this a constructor?
Both subclasses call it first thing in the constructor.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:307: ..state = state
nit: indentation. (by 4).

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:314: /// The recursive
continuation builder has fresh parameters (i.e. SSA phis)
"i.e." doesn't seem right. It's more of a "like" no?

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:339: void beginField({
ClosureScope closureScope }) {
nit: apparently we don't put spaces after "{" and before "}". (same for other
functions.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:339: void beginField({
ClosureScope closureScope }) {
comment.
Is this the rhs of a field-init?

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:343: void
beginFunction(Iterable<ParameterElement> parameters,
Apparently this method is not just used for functions (see call from
beginField).

I think it would be safer to factor out the shared part (altough I'm missing a
good name right now).

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:347:
_enterClosureEnvironment(closureEnvironment);
unconditionally _enterClosureEnvironment(...) and test in there if the argument
is `null`.

rename to _buildClosureEnvironmentSetup(...); ?

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:350: _enterScope(closureScope);
ditto.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:355: /// Create a parameter for
[local] and add it to the current environment.
Creates ... adds ...

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:363: /// Add the constant
[variableElement] to the environment with [value] as its
Adds

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:862: ClosureScope closureScope})
{
add comment about closureScope variable.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:888: _enterScope(closureScope);
as above (and for the remaining ones): unconditionally call and test in the
function.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:921:
updateBuilder._migrateLoopVariables(closureScope);
ditto

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1545: /// True if [local] is
stored in a ref cell.
What's a "ref cell" ?
Explain more.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1643: /// Inner functions are
repsented by a [ClosureClassElement], and captured
represented

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1654: ir.Primitive thisPrim =
new ir.This();
more comments.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1660: ir.Primitive load = new
ir.GetField(thisPrim, location.field);
Add comment, that dead-code elimination will remove unnecessary loads.
(correct?)

https://codereview.chromium.org/831133004/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/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:158:
map.forEach((key,value) {
indentation and missing space.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:168: return new
ClosureLocation(null, v as ClosureFieldElement);
It's stupid but we avoid `as`.
Just create a temporary and assign to it.
(unless you don't need the `as` which should be likely)

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:173: if (closureMap ==
null) return null; // dart2dart does not use closureMap
Finish comment with ".".

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:181: ClosureScope
getClosureScope(ast.Node node) {
Add lots of documentation.
I wrote the closure.dart file and it still took me some time to remember what
these scopes are.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:182: if (closureMap ==
null) return null; // dart2dart does not use closureMap
ditto.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:224:
builder.beginField(closureScope: getClosureScope(fieldDefinition));
I would rename this call to something like buildFieldExpressionHeader(...)
This makes it clearer that we are actually generating something.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:240:
irBuilder.beginFunction(parameters,
ditto.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:403: if ((irBuilder as
DartIrBuilder).isInRefCell(element)) {
ditto: we don't use `as`.
Just assign to a temporary before:
DartIrBuilder dartIrBuilder = irBuilder;
if (dartIrBuilder.isInRefCell(element)) { ... }

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:1064: Set<Local>
capturedVariables = new Set<Local>();
nit: maybe go directly for a Setlet ?

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart (right):

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart:289: if
(SubexpressionVisitor.usesInfixNotation(node.object)) {
Does this capture `(-x).abs` ?

https://codereview.chromium.org/831133004/diff/1/tests/language/for_variable_...
File tests/language/for_variable_capture_test.dart (right):

https://codereview.chromium.org/831133004/diff/1/tests/language/for_variable_...
tests/language/for_variable_capture_test.dart:11: var closure;
indentation (and in rest of file).

[sigurdm, 2015-01-09 09:31:21]

LGTM

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

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:30: /// A map from elements to
their environment index.
Perhaps update comment

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:241: void
declareLocalVariable(LocalVariableElement element, {ir.Primitive initialValue});
Long line

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:306: return _makeInstance()
Perhaps make-instance should take parameters

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:350: _enterScope(closureScope);
Maybe _enterScope should be a nop on the dart-builder, - then you would not have
to check for closureScope != null.

Similarly for _enterClosureEnvironment and _migrateLoopVariables

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1410: /// If inside a closure
class, [buildThis] will redirect acess through closure
acess -> access

https://codereview.chromium.org/831133004/diff/20001/tests/language/for_varia...
File tests/language/for_variable_capture_test.dart (right):

https://codereview.chromium.org/831133004/diff/20001/tests/language/for_varia...
tests/language/for_variable_capture_test.dart:11: var closure;
Use 2-space indent

https://codereview.chromium.org/831133004/diff/20001/tests/language/for_varia...
tests/language/for_variable_capture_test.dart:111: Expect.listEquals(   [2, 3,
4, 5], update_closures.map(run).toList());
I don't like this whitespace. Maybe a newline after the update line could
highlight the grouping.

[asgerf, 2015-01-12 13:15:44]

Response to comments.

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

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:183: class IrBuilderClosureState
{
On 2015/01/08 18:29:35, floitsch wrote:
> Probably different CL, but I'm not sure the names are really good:
> the state that is active for all nested functions is the "ClosureState", but
the
> state that is active for just my local function (aka "closure") is called
> "SharedState".

I completely agree, but I can't seem to think of better names.
SharedClosureState and SharedLocalState? It still seems to imply that
ClosureState is bound to one closure as opposed to shared across closure
boundaries.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:228: /// If non-null, this
refers to the receiver in the enclosing method.
On 2015/01/08 18:29:35, floitsch wrote:
> to the receiver (`this`) in ...

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:293: void _init(ConstantSystem
constantSystem, ExecutableElement currentElement) {
On 2015/01/08 18:29:36, floitsch wrote:
> add comment.
> 
> Could you keep this a constructor?
> Both subclasses call it first thing in the constructor.

Added comment.

The subclasses have two constructors, because the makeInstance() should not do
this initialization. Duplicating the code for makeDelimitedBuilder,
makeRecursiveBuilder and for _init seems a bit much.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:314: /// The recursive
continuation builder has fresh parameters (i.e. SSA phis)
On 2015/01/08 18:29:35, floitsch wrote:
> "i.e." doesn't seem right. It's more of a "like" no?

Parameters are like SSA phis, but using "like" here I think could easily be
misinterpreted as "for example".

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:339: void beginField({
ClosureScope closureScope }) {
On 2015/01/08 18:29:36, floitsch wrote:
> nit: apparently we don't put spaces after "{" and before "}". (same for other
> functions.

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:343: void
beginFunction(Iterable<ParameterElement> parameters,
On 2015/01/08 18:29:36, floitsch wrote:
> Apparently this method is not just used for functions (see call from
> beginField).
> 
> I think it would be safer to factor out the shared part (altough I'm missing a
> good name right now).

Fixed.

The only shared part was _enterScope, so I just copied that into beginField.
With the null check now inside _enterScope, there is no real code duplication
anyway.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:347:
_enterClosureEnvironment(closureEnvironment);
On 2015/01/08 18:29:35, floitsch wrote:
> unconditionally _enterClosureEnvironment(...) and test in there if the
argument
> is `null`.
> 
> rename to _buildClosureEnvironmentSetup(...); ?

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:350: _enterScope(closureScope);
On 2015/01/08 18:29:35, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:355: /// Create a parameter for
[local] and add it to the current environment.
On 2015/01/08 18:29:36, floitsch wrote:
> Creates ... adds ...

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:363: /// Add the constant
[variableElement] to the environment with [value] as its
On 2015/01/08 18:29:35, floitsch wrote:
> Adds

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1545: /// True if [local] is
stored in a ref cell.
On 2015/01/08 18:29:35, floitsch wrote:
> What's a "ref cell" ?
> Explain more.

Changed to isInClosureVariable. I was thinking about renaming ClosureVariable to
RefCell but then changed my mind halfway through.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1643: /// Inner functions are
repsented by a [ClosureClassElement], and captured
On 2015/01/08 18:29:36, floitsch wrote:
> represented

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1654: ir.Primitive thisPrim =
new ir.This();
On 2015/01/08 18:29:36, floitsch wrote:
> more comments.

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1660: ir.Primitive load = new
ir.GetField(thisPrim, location.field);
On 2015/01/08 18:29:35, floitsch wrote:
> Add comment, that dead-code elimination will remove unnecessary loads.
> (correct?)

Shrinking reductions will remove a dead GetField, but I think the comment would
be misleading.

Assuming [freeVariables] does not have superfluous entries, then unless the free
occurrence is unreachable control-flow wise, there will be no unnecessary loads.
And besides, there is no check we can reasonably perform here that would prevent
creating such a GetField.

https://codereview.chromium.org/831133004/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/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:158:
map.forEach((key,value) {
On 2015/01/08 18:29:36, floitsch wrote:
> indentation and missing space.

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:168: return new
ClosureLocation(null, v as ClosureFieldElement);
On 2015/01/08 18:29:37, floitsch wrote:
> It's stupid but we avoid `as`.
> Just create a temporary and assign to it.
> (unless you don't need the `as` which should be likely)

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:173: if (closureMap ==
null) return null; // dart2dart does not use closureMap
On 2015/01/08 18:29:37, floitsch wrote:
> Finish comment with ".".

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:181: ClosureScope
getClosureScope(ast.Node node) {
On 2015/01/08 18:29:36, floitsch wrote:
> Add lots of documentation.
> I wrote the closure.dart file and it still took me some time to remember what
> these scopes are.

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:182: if (closureMap ==
null) return null; // dart2dart does not use closureMap
On 2015/01/08 18:29:37, floitsch wrote:
> ditto.

Done.

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart (right):

https://codereview.chromium.org/831133004/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart:289: if
(SubexpressionVisitor.usesInfixNotation(node.object)) {
On 2015/01/08 18:29:37, floitsch wrote:
> Does this capture `(-x).abs` ?

The IR tracer does not currently use unary pre/postfix notation for anything.

https://codereview.chromium.org/831133004/diff/1/tests/language/for_variable_...
File tests/language/for_variable_capture_test.dart (right):

https://codereview.chromium.org/831133004/diff/1/tests/language/for_variable_...
tests/language/for_variable_capture_test.dart:11: var closure;
On 2015/01/08 18:29:37, floitsch wrote:
> indentation (and in rest of file).

Done.

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

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:30: /// A map from elements to
their environment index.
On 2015/01/09 09:31:20, sigurdm wrote:
> Perhaps update comment

Done.

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:241: void
declareLocalVariable(LocalVariableElement element, {ir.Primitive initialValue});
On 2015/01/09 09:31:20, sigurdm wrote:
> Long line

Done.

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:306: return _makeInstance()
On 2015/01/09 09:31:20, sigurdm wrote:
> Perhaps make-instance should take parameters

The fields are declared in this class, not the subclass, so it makes to
initialize them here.

I guess the old-school OO dogma says we should send the arguments into
makeInstance(), then into a subclass constructor, and then use super() to pass
them back to an IrBuilder constructor.

That would guarantee that we did not forget to initialize a final field (unless
we used named arguments), but the amount of boilerplate is not worth it in my
experience. So I did it like this.

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:350: _enterScope(closureScope);
On 2015/01/09 09:31:20, sigurdm wrote:
> Maybe _enterScope should be a nop on the dart-builder, - then you would not
have
> to check for closureScope != null.
> 
> Similarly for _enterClosureEnvironment and _migrateLoopVariables

I don't entirely agree, but OK. floitch had the same comment.

https://codereview.chromium.org/831133004/diff/20001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:1410: /// If inside a closure
class, [buildThis] will redirect acess through closure
On 2015/01/09 09:31:20, sigurdm wrote:
> acess -> access

Thanks.

https://codereview.chromium.org/831133004/diff/20001/tests/language/for_varia...
File tests/language/for_variable_capture_test.dart (right):

https://codereview.chromium.org/831133004/diff/20001/tests/language/for_varia...
tests/language/for_variable_capture_test.dart:111: Expect.listEquals(   [2, 3,
4, 5], update_closures.map(run).toList());
On 2015/01/09 09:31:20, sigurdm wrote:
> I don't like this whitespace. Maybe a newline after the update line could
> highlight the grouping.

Good idea, the newline seems to help. Made some other adjustments as well to
make the arrangement clear.

[Kevin Millikin (Google), 2015-01-13 08:25:37]

LGTM if the comments are addressed.

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

https://codereview.chromium.org/831133004/diff/60001/pkg/analyzer2dart/lib/sr...
pkg/analyzer2dart/lib/src/cps_generator.dart:69: element,
The arguments were aligned before, now they're offside.  I suggest just
indenting them 4 spaces.

https://codereview.chromium.org/831133004/diff/60001/pkg/analyzer2dart/lib/sr...
pkg/analyzer2dart/lib/src/cps_generator.dart:84: element,
Same: 4-space indent.

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/clo...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/clo...
pkg/compiler/lib/src/closure.dart:339: // their captured copies.
'captured copies' seems bad.  How about just 'locations'.  (CapturedVariable
seems like the wrong name too, but let's save renaming for a separate change if
we want it.)

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/clo...
pkg/compiler/lib/src/closure.dart:343: // Maps scopes ([Loop] and
[FunctionExpression] nodes) to their
'scopes' ==> 'nested scopes' or 'inner scopes' or the like.

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

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:465: class SetField extends
Expression implements JsSpecificNode {
Also implements InteriorNode (a node with a body).

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

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:236: return
'$indentation(SetField $object.$field = $value in\n$body)';
I don't really want '.', '=', or 'in' here.  This representation is abstract
syntax, and they just make parsing harder.

Though the expression has a body, it's not really a scope (setting a field is
assignment, not binding).  I'd prefer it formatted like setq in Lisp or set! in
Scheme:

String body = visit(node.body);
return '$indentation(SetField $object $field $value)\n$body';

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:242: return '(GetField
$object.$field)';
No dot:

'(GetField $object $field)'

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:252: return
'(CreateClosureClass $className $arguments)';
'(CreateClosureClass $className ($arguments))'

[asgerf, 2015-01-13 10:04:26]

https://codereview.chromium.org/831133004/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/831133004/diff/1/pkg/compiler/lib/src/cps_ir/...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_visitor.dart:224:
builder.beginField(closureScope: getClosureScope(fieldDefinition));
On 2015/01/08 18:29:37, floitsch wrote:
> I would rename this call to something like buildFieldExpressionHeader(...)
> This makes it clearer that we are actually generating something.

Everything in this class is already randomly prefixed with build/create/make so
why not...

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

https://codereview.chromium.org/831133004/diff/60001/pkg/analyzer2dart/lib/sr...
pkg/analyzer2dart/lib/src/cps_generator.dart:69: element,
On 2015/01/13 08:25:36, kmillikin wrote:
> The arguments were aligned before, now they're offside.  I suggest just
> indenting them 4 spaces.

Thanks

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/clo...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/clo...
pkg/compiler/lib/src/closure.dart:339: // their captured copies.
On 2015/01/13 08:25:36, kmillikin wrote:
> 'captured copies' seems bad.  How about just 'locations'.  (CapturedVariable
> seems like the wrong name too, but let's save renaming for a separate change
if
> we want it.)

Done.

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/clo...
pkg/compiler/lib/src/closure.dart:343: // Maps scopes ([Loop] and
[FunctionExpression] nodes) to their
On 2015/01/13 08:25:36, kmillikin wrote:
> 'scopes' ==> 'nested scopes' or 'inner scopes' or the like.

I just removed 'scope'.

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

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:465: class SetField extends
Expression implements JsSpecificNode {
On 2015/01/13 08:25:36, kmillikin wrote:
> Also implements InteriorNode (a node with a body).

Yikes. Thanks for catching that.

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

https://codereview.chromium.org/831133004/diff/60001/pkg/compiler/lib/src/cps...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:236: return
'$indentation(SetField $object.$field = $value in\n$body)';
On 2015/01/13 08:25:36, kmillikin wrote:
> I don't really want '.', '=', or 'in' here.  This representation is abstract
> syntax, and they just make parsing harder.
> 
> Though the expression has a body, it's not really a scope (setting a field is
> assignment, not binding).  I'd prefer it formatted like setq in Lisp or set!
in
> Scheme:
> 
> String body = visit(node.body);
> return '$indentation(SetField $object $field $value)\n$body';

Yeah, I must have been half-thinking about the IR tracer when I wrote this.

[asgerf, 2015-01-13 11:57:36]

Committed patchset #7 (id:120001) manually as 42802 (presubmit successful).