Hopefully the last bit of restructuring between closture classes and loop boxing, etc.

Hopefully the last bit of restructuring between closture classes and loop boxing, etc.

BUG=
R=johnniwinther@google.com, sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/6960d77fbc5df8154b2e5b1187606b325bd4fbb7

[Emily Fortuna, 2017-06-26 00:45:27]

efortuna@google.com changed reviewers:
+ johnniwinther@google.com, sigmund@google.com

[Johnni Winther, 2017-06-26 08:31:39]

lgtm

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:89: /// Loop through each variabe that has
been defined in this scope, modified
`variabe` -> `variable`

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:90: /// anywhere (this scope or another scope)
and used in another scope . Because
`scope .` -> `scope.`

Start a new paragraph at`Because`

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:104: /// True if this loop declares any
variables that need to be boxed.
`this loop` -> `this scope`

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:633: capturedVariables.forEach(f);
assert(boxedLoopVariables.isEmpty)

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:643: Map<Local, BoxFieldElement> get
boxedVariables {
When is this used?

[Siggi Cherem (dart-lang), 2017-06-26 16:54:43]

lgtm, just some suggestions below

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:616: class ClosureScope implements ClosureBase
{
mmm... I sort of like the idea of swapping the two names: make ClosureScope the
public interface and ClosureBase the base implementation for
ClosureRepresentationInfo.

Thoughts?

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/pubspec.yaml
File pkg/compiler/pubspec.yaml (right):

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/pubspec.yaml#n...
pkg/compiler/pubspec.yaml:8: collection: '>=1.14.1'
delete? (I didn't see this used elsewhere)

https://codereview.chromium.org/2961563003/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization/model_test_helper.dart (left):

https://codereview.chromium.org/2961563003/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization/model_test_helper.dart:267:
checkSets(a.boxedLoopVariables, b.boxedLoopVariables,
should we replace this somehow?

One option would be to create 2 maps locally and then compare them:

var aBoxed = {};
a.forEachBoxedVariable((k, v) => aBoxed[k] = v);
var bBoxed = {};
b.forEachBoxedVariable((k, v) => bBoxed[k] = v);

checkMaps(aBoxed, bBoxed, ...)

[Emily Fortuna, 2017-06-26 21:57:20]

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:89: /// Loop through each variabe that has
been defined in this scope, modified
On 2017/06/26 08:31:39, Johnni Winther wrote:
> `variabe` -> `variable`

Done.

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:90: /// anywhere (this scope or another scope)
and used in another scope . Because
On 2017/06/26 08:31:39, Johnni Winther wrote:
> `scope .` -> `scope.`
> 
> Start a new paragraph at`Because`

Done.

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:104: /// True if this loop declares any
variables that need to be boxed.
On 2017/06/26 08:31:39, Johnni Winther wrote:
> `this loop` -> `this scope`

thank you!

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/lib/src/closur...
pkg/compiler/lib/src/closure.dart:643: Map<Local, BoxFieldElement> get
boxedVariables {
On 2017/06/26 08:31:39, Johnni Winther wrote:
> When is this used?

oops. that was holdover from a previous edit. Removed!

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/pubspec.yaml
File pkg/compiler/pubspec.yaml (right):

https://codereview.chromium.org/2961563003/diff/1/pkg/compiler/pubspec.yaml#n...
pkg/compiler/pubspec.yaml:8: collection: '>=1.14.1'
On 2017/06/26 16:54:42, Siggi Cherem (dart-lang) wrote:
> delete? (I didn't see this used elsewhere)

ugh, yes sorry. git branch fail.

https://codereview.chromium.org/2961563003/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization/model_test_helper.dart (left):

https://codereview.chromium.org/2961563003/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization/model_test_helper.dart:267:
checkSets(a.boxedLoopVariables, b.boxedLoopVariables,
On 2017/06/26 16:54:42, Siggi Cherem (dart-lang) wrote:
> should we replace this somehow?
> 
> One option would be to create 2 maps locally and then compare them:
> 
> var aBoxed = {};
> a.forEachBoxedVariable((k, v) => aBoxed[k] = v);
> var bBoxed = {};
> b.forEachBoxedVariable((k, v) => bBoxed[k] = v);
> 
> checkMaps(aBoxed, bBoxed, ...)

Done.

[Siggi Cherem (dart-lang), 2017-06-26 22:42:35]

lgtm

[Emily Fortuna, 2017-06-26 22:49:31]

Description was changed from

==========
Hopefully the last bit of restructuring between closture classes and loop
boxing, etc.

BUG=
==========

to

==========
Hopefully the last bit of restructuring between closture classes and loop
boxing, etc.

BUG=
R=johnniwinther@google.com, sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6960d77fbc5df8154b2e5b1187606b325bd4fbb7
==========

[Emily Fortuna, 2017-06-26 22:49:32]

Committed patchset #2 (id:20001) manually as
6960d77fbc5df8154b2e5b1187606b325bd4fbb7 (presubmit successful).