Remove unnecessary contexts in closure conversion

Remove unnecessary contexts in closure conversion

R=karlklose@google.com

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

[Dmitry Stefantsov, 2017-06-15 12:44:17]

dmitryas@google.com changed reviewers:
+ ahe@google.com, karlklose@google.com

[Dmitry Stefantsov, 2017-06-15 12:44:18]

This CL removes some unnecessary contexts that sometimes are introduced by
closure conversion.  The reason they were created is that 'currentFunction'
field of 'ClosureInfo' file was unset while visiting constructor initializers
which means that the constructor parameters are treated as captured (function
that they are declared at is not 'currentFunction').  So, the solution is to set
'currentFunction' while visiting initializers to the FunctionNode of the
constructor.

[karlklose, 2017-06-16 07:54:43]

https://codereview.chromium.org/2939043002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2939043002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:217:
visitFieldInitializer(FieldInitializer node) {
Consider adding a helper and move the comment there.

The comment could also be shorter; maybe adding an example could help.

[Dmitry Stefantsov, 2017-06-16 08:30:00]

Thanks for the review, Karl!  I implemented the suggested changes.  PTAL.

https://codereview.chromium.org/2939043002/diff/20001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2939043002/diff/20001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:217:
visitFieldInitializer(FieldInitializer node) {
On 2017/06/16 07:54:43, karlklose wrote:
> Consider adding a helper and move the comment there.
> 
> The comment could also be shorter; maybe adding an example could help.

Thanks!  I agree, an example could help with understanding of this change.  Also
agree on the helper function.

One more thing that I changed is that I save/restore currentFunction for the
constructor, not individual initializers.  It's probably more clear and saves
some resources.

[Dmitry Stefantsov, 2017-06-16 08:36:28]

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:114: //       A(int x)  /* "x"
is visible in initializers and body. */
I think I missed a spot here.  "x" should have been _x_ in the comment.  But,
probably, it's better to use `x` here and below to describe parts of the
example.

[karlklose, 2017-06-16 08:43:41]

LGTM

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:114: //       A(int x)  /* "x"
is visible in initializers and body. */
On 2017/06/16 08:36:27, Dmitry Stefantsov wrote:
> I think I missed a spot here.  "x" should have been _x_ in the comment.  But,
> probably, it's better to use `x` here and below to describe parts of the
> example.

Consider using '///' and '[x]' here and below (although you are technically in a
level two comment)

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:120: // Here _x_ parameter
shouldn't be captured into a context in the
"_x_ parameter" -> "the parameter"?

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:121: // initializer.  However,
if [currentFunction] is not set, it has _null_
How about 'However, [currentFunction] is `null` if not set, ...'

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:123: // [FunctionNode] of the
constructor (which is not _null_).  It leads to "x"
`null`

[Dmitry Stefantsov, 2017-06-16 08:55:28]

Thanks for the suggestions, Karl!

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
File pkg/kernel/lib/transformations/closure/info.dart (right):

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:114: //       A(int x)  /* "x"
is visible in initializers and body. */
On 2017/06/16 08:43:40, karlklose wrote:
> On 2017/06/16 08:36:27, Dmitry Stefantsov wrote:
> > I think I missed a spot here.  "x" should have been _x_ in the comment. 
But,
> > probably, it's better to use `x` here and below to describe parts of the
> > example.
> 
> Consider using '///' and '[x]' here and below (although you are technically in
a
> level two comment)

Done.

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:120: // Here _x_ parameter
shouldn't be captured into a context in the
On 2017/06/16 08:43:41, karlklose wrote:
> "_x_ parameter" -> "the parameter"?

Done.

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:121: // initializer.  However,
if [currentFunction] is not set, it has _null_
On 2017/06/16 08:43:41, karlklose wrote:
> How about 'However, [currentFunction] is `null` if not set, ...'

Agree, that reads much better.

https://codereview.chromium.org/2939043002/diff/40001/pkg/kernel/lib/transfor...
pkg/kernel/lib/transformations/closure/info.dart:123: // [FunctionNode] of the
constructor (which is not _null_).  It leads to "x"
On 2017/06/16 08:43:41, karlklose wrote:
> `null`

Done.

[Dmitry Stefantsov, 2017-06-16 09:01:17]

Description was changed from

==========
Remove unnecessary contexts in closure conversion
==========

to

==========
Remove unnecessary contexts in closure conversion

R=karlklose@google.com

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

[Dmitry Stefantsov, 2017-06-16 09:01:19]

Committed patchset #6 (id:100001) manually as
6ebe771a71a485fdeb6bf4f52adf350ace33cb91 (presubmit successful).