Reapply "Added for-loop variable tracking and regular closures/initializers captured variable track…

Reapply "Added for-loop variable tracking and regular closures/initializers captured variable tracking."

This reverts commit b6671a0e3f6904edc1a1aa787fc8a13bce95436f.

BUG=
R=sigmund@google.com

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

[Emily Fortuna, 2017-07-06 18:21:11]

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

[Emily Fortuna, 2017-07-06 18:21:12]

The first patchset is the previously committed version. The new patchset is my
changes, plus a little bit of rebasing. Sorry for the noise....

[Siggi Cherem (dart-lang), 2017-07-06 19:15:32]

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_model/closure_visitors.dart (right):

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:108:
ir.VariableDeclaration>[]; // TODO: need to construct a new _curentScopeInfo as
we... enter a new scope! or do we do it earlier because the for loop calls this
method too.
nit: 80 col :)

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:159: print('we add variable
$variable');
delete

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:239: print(
ditto

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:253: while (node.parent !=
null) {
did you find cases where the node is not under the outermostNode?

It should hopefully be safe to combine the two conditions too.

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:258: /*while (node !=
_outermostNode) {
delete?

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:282: if (nodeToConvert is
ir.Field && nodeToConvert.isInstanceMember) {
Maybe split in two conditions, so you can return quickly when the field is not
an instance member?

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:301: }
if we have coverage above for all the situations, could we throw here with an
unexpected error (e.g. unsupported TreeNode?)

[Emily Fortuna, 2017-07-06 19:29:41]

ptal

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_model/closure_visitors.dart (right):

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:108:
ir.VariableDeclaration>[]; // TODO: need to construct a new _curentScopeInfo as
we... enter a new scope! or do we do it earlier because the for loop calls this
method too.
On 2017/07/06 19:15:32, Siggi Cherem (dart-lang) wrote:
> nit: 80 col :)

oh geez. sorry. I clearly didn't take out all my debug stuff. :-P

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:159: print('we add variable
$variable');
On 2017/07/06 19:15:32, Siggi Cherem (dart-lang) wrote:
> delete

Done.

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:239: print(
On 2017/07/06 19:15:32, Siggi Cherem (dart-lang) wrote:
> ditto

Done.

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:253: while (node.parent !=
null) {
On 2017/07/06 19:15:31, Siggi Cherem (dart-lang) wrote:
> did you find cases where the node is not under the outermostNode?
> 
> It should hopefully be safe to combine the two conditions too.

Done.

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:258: /*while (node !=
_outermostNode) {
On 2017/07/06 19:15:32, Siggi Cherem (dart-lang) wrote:
> delete?

Done.

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:282: if (nodeToConvert is
ir.Field && nodeToConvert.isInstanceMember) {
On 2017/07/06 19:15:32, Siggi Cherem (dart-lang) wrote:
> Maybe split in two conditions, so you can return quickly when the field is not
> an instance member?

Done.

https://codereview.chromium.org/2964783002/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:301: }
On 2017/07/06 19:15:31, Siggi Cherem (dart-lang) wrote:
> if we have coverage above for all the situations, could we throw here with an
> unexpected error (e.g. unsupported TreeNode?)

there's an else inside that other else, so it really should be completely
impossible to reach this point.

I changed it so that we return null by hitting this point (which is what the
else inside the else did)

[Siggi Cherem (dart-lang), 2017-07-06 19:35:39]

lgtm

[Emily Fortuna, 2017-07-06 21:09:58]

Description was changed from

==========
Reapply "Added for-loop variable tracking and regular closures/initializers
captured variable tracking."

This reverts commit b6671a0e3f6904edc1a1aa787fc8a13bce95436f.

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

to

==========
Reapply "Added for-loop variable tracking and regular closures/initializers
captured variable tracking."

This reverts commit b6671a0e3f6904edc1a1aa787fc8a13bce95436f.

BUG=
R=sigmund@google.com

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

[Emily Fortuna, 2017-07-06 21:09:59]

Committed patchset #4 (id:60001) manually as
f934f93758aa4a09289473f2fa702505806a4894 (presubmit successful).