Add ClosureRepresentationInfo, the new public face of ClosureClassMap

Add ClosureRepresentationInfo, the new public face of ClosureClassMap
(removal of methods exposing ClosureClassMap coming in follow up CL).

BUG=
R=johnniwinther@google.com

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

[Emily Fortuna, 2017-06-14 00:54:22]

Patchset #2 (id:20001) has been deleted

[Emily Fortuna, 2017-06-14 00:54:38]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2017-06-14 00:54:53]

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

[Johnni Winther, 2017-06-14 09:22:30]

lgtm

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:31: ClosureClassMap getMemberMap(MemberEntity
member);
Do we still need these?

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:36: ClosureRepresentationInfo
getClosureRepresentationInfo(Entity member);
Add a TODO for me to split this into members and local functions (like
getMemberMap/getLocalFunctionMap)

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:125: /// I expect this interface to get
simpler in subsequent refactorings.
Maybe turn the last line into a TODO?

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:134: /// Closures are rewritten in the form of
classes that
Weird line break

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:143: FunctionEntity get callEntity => null;
Maybe rename to [callMethod]?

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:183: void forEachFreeVariable(f(Local
variable, FieldEntity field)) {}
Add docs

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:185: /// Return true if [variable] has been
captured and mutated (other variables
(...) -> , if not [variable] doesn't need boxing.

[Emily Fortuna, 2017-06-14 18:15:13]

Description was changed from

==========
Add ClosureRepresentationInfo, the new public face of ClosureClassMap
(removal of methods exposing ClosureClassMap coming in follow up CL).

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

to

==========
Add ClosureRepresentationInfo, the new public face of ClosureClassMap
(removal of methods exposing ClosureClassMap coming in follow up CL).

BUG=
R=johnniwinther@google.com

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

[Emily Fortuna, 2017-06-14 18:15:14]

Committed patchset #2 (id:60001) manually as
a27ebd0c949a37a43069445d26c195989adab9e8 (presubmit successful).

[Siggi Cherem (dart-lang), 2017-06-14 22:56:36]

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

[Siggi Cherem (dart-lang), 2017-06-14 22:56:36]

lgtm, nice!

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

https://codereview.chromium.org/2933363003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:101: /// Class that describes the actual
mechanics of how the converted, rewritten
brainstorming here: in a way I think "convertion/rewriting" is the
implementation :), so I wonder about what noun we should use to refer to the
rewritten closure. I'm not opposed to keep it as "rewritten closure " like you
have it, just thinking of ideas. For example, other ideas:
- emitter representation of Dart closures
- JavaScript representation of a Dart closure
- closure's implementation representation.

If we change the terminology, I'd consider updating the docs, for example, this
sentence here could be:

Class that describes the actual mechanics of how closures are implemented. This
includes information about how the closure is represented in the emitted
JavaScript in terms of classes and fields. ....

https://codereview.chromium.org/2933363003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:133: /// Closures are rewritten in the form of
classes that have fields to control
minor nit: consider moving up the sentence that just describes what this
property is (since there is context already in the class comment), and then add
more details. For example:

/// The entity for the class used to represent the closure in the emitted
JavaScript.
///
/// Closures are represented as a class, where each field represents a captured
variable.
/// **A capture variable is a variable declared in an scope outside the closure
and used in an 
/// scope within the closure.

** - I'd probably not include the definition of captured varaibles here, but
move it to the class comment and other methods below that use the term.

[Emily Fortuna, 2017-06-15 00:11:22]

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:31: ClosureClassMap getMemberMap(MemberEntity
member);
On 2017/06/14 09:22:30, Johnni Winther wrote:
> Do we still need these?

no, but I was going to delete them in a follow-up CL.

but I went ahead and did it here.

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:36: ClosureRepresentationInfo
getClosureRepresentationInfo(Entity member);
On 2017/06/14 09:22:30, Johnni Winther wrote:
> Add a TODO for me to split this into members and local functions (like
> getMemberMap/getLocalFunctionMap)

Done.

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:125: /// I expect this interface to get
simpler in subsequent refactorings.
On 2017/06/14 09:22:30, Johnni Winther wrote:
> Maybe turn the last line into a TODO?

Done.

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:134: /// Closures are rewritten in the form of
classes that
On 2017/06/14 09:22:30, Johnni Winther wrote:
> Weird line break

Done.

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:143: FunctionEntity get callEntity => null;
On 2017/06/14 09:22:30, Johnni Winther wrote:
> Maybe rename to [callMethod]?

Done.

https://codereview.chromium.org/2933363003/diff/40001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:183: void forEachFreeVariable(f(Local
variable, FieldEntity field)) {}
On 2017/06/14 09:22:30, Johnni Winther wrote:
> Add docs

Done.

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

https://codereview.chromium.org/2933363003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:101: /// Class that describes the actual
mechanics of how the converted, rewritten
On 2017/06/14 22:56:36, Siggi Cherem (dart-lang) wrote:
> brainstorming here: in a way I think "convertion/rewriting" is the
> implementation :), so I wonder about what noun we should use to refer to the
> rewritten closure. I'm not opposed to keep it as "rewritten closure " like you
> have it, just thinking of ideas. For example, other ideas:
> - emitter representation of Dart closures
> - JavaScript representation of a Dart closure
> - closure's implementation representation.
> 
> If we change the terminology, I'd consider updating the docs, for example,
this
> sentence here could be:
> 
> Class that describes the actual mechanics of how closures are implemented.
This
> includes information about how the closure is represented in the emitted
> JavaScript in terms of classes and fields. ....

"Closure Conversion" is the standard term used to describe the process, so I'm
inclined to keep it. Can discuss more, tho.

https://codereview.chromium.org/2933363003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:133: /// Closures are rewritten in the form of
classes that have fields to control
On 2017/06/14 22:56:36, Siggi Cherem (dart-lang) wrote:
> minor nit: consider moving up the sentence that just describes what this
> property is (since there is context already in the class comment), and then
add
> more details. For example:
> 
> /// The entity for the class used to represent the closure in the emitted
> JavaScript.
> ///
> /// Closures are represented as a class, where each field represents a
captured
> variable.
> /// **A capture variable is a variable declared in an scope outside the
closure
> and used in an 
> /// scope within the closure.
> 
> ** - I'd probably not include the definition of captured varaibles here, but
> move it to the class comment and other methods below that use the term.

Cool. I updated the docs in this CL: https://codereview.chromium.org/2937143002