It's alive!

It's alive!

With this change the empty program compiles and runs from .dill

R=sigmund@google.com

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

[Johnni Winther, 2017-05-30 11:26:29]

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

[Siggi Cherem (dart-lang), 2017-05-30 22:10:20]

lgtm! WOOHOO! \o/  Yippie!!

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/common...
File pkg/compiler/lib/src/common_elements.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common_elements.dart:1156: /// The _env makes private and
injected members directly available and
unrelated, but we might want to fix this comment:
 - `_env` => This class?
 - mention that it provides APIs also to inspect properties of
libraries/classes/members?
 - mention why some things are properties in the model and why some things are
here. For example, why isUnnamedMixin is not defined in ClassEntity?

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:190: if (!isProgramSplit) return
mainOutputUnit;
how about removing the check here and the cast below, instead do change
outputUnitForElement to take an Entity, return quickly and then cast to an
Element?

if (!isProgramSplit) return ...;
// TODO(johhniwinther): support entities by .... 
Element element = entity;

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:550: static String
reconstructConstructorName(FunctionEntity element) {
In the last CL there was a place in namer.dart where you inlined this code.
Maybe we can undo that now?

Also - should we move these two functions elsewhere (not Elements, but somewhere
else?)

In the past I've been told a lot to avoid container classes with plural names
like this one, so my preference would be to make these top-level methods in a
helper library, maybe: elements/entity_utils.dart ?

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/namer.dart:776: if (method is
ConstructorBodyElement) {
we'll need this for the J model too, correct? Maybe add a TODO (since we'll
silently skip this line as we start adding the J model in the future?

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/full_emitter/class_builder.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/full_emitter/class_builder.dart:25: final bool
isForActualClass;
ha - I haven't seen this yet... seems like a sign that we have a misnomer for
this class...

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:1269: void
checkEverythingEmitted(Map<ClassEntity, ClassBuilder> classes) {
nit: classes => pendingClassBuilders
     pendingStatics => pendingClasses
?

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart:230: if
(member.enclosingClass != null && member.enclosingClass.isClosure) {
I just noticed that we added `isClosure` to classEntity - do we want to move
away from that in the long run?)

[Johnni Winther, 2017-05-31 08:19:31]

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/common...
File pkg/compiler/lib/src/common_elements.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common_elements.dart:1156: /// The _env makes private and
injected members directly available and
On 2017/05/30 22:10:20, Siggi Cherem (dart-lang) wrote:
> unrelated, but we might want to fix this comment:
>  - `_env` => This class?
>  - mention that it provides APIs also to inspect properties of
> libraries/classes/members?
>  - mention why some things are properties in the model and why some things are
> here. For example, why isUnnamedMixin is not defined in ClassEntity?

Added a bit...

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/deferr...
File pkg/compiler/lib/src/deferred_load.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/deferr...
pkg/compiler/lib/src/deferred_load.dart:190: if (!isProgramSplit) return
mainOutputUnit;
On 2017/05/30 22:10:20, Siggi Cherem (dart-lang) wrote:
> how about removing the check here and the cast below, instead do change
> outputUnitForElement to take an Entity, return quickly and then cast to an
> Element?
> 
> if (!isProgramSplit) return ...;
> // TODO(johhniwinther): support entities by .... 
> Element element = entity;

Done.

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/elemen...
File pkg/compiler/lib/src/elements/elements.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/elemen...
pkg/compiler/lib/src/elements/elements.dart:550: static String
reconstructConstructorName(FunctionEntity element) {
On 2017/05/30 22:10:20, Siggi Cherem (dart-lang) wrote:
> In the last CL there was a place in namer.dart where you inlined this code.
> Maybe we can undo that now?
> 
> Also - should we move these two functions elsewhere (not Elements, but
somewhere
> else?)
> 
> In the past I've been told a lot to avoid container classes with plural names
> like this one, so my preference would be to make these top-level methods in a
> helper library, maybe: elements/entity_utils.dart ?

Done (for the former), added a TODO (for the latter)

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/namer.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/namer.dart:776: if (method is
ConstructorBodyElement) {
On 2017/05/30 22:10:20, Siggi Cherem (dart-lang) wrote:
> we'll need this for the J model too, correct? Maybe add a TODO (since we'll
> silently skip this line as we start adding the J model in the future?

Done.

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/full_emitter/class_builder.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/full_emitter/class_builder.dart:25: final bool
isForActualClass;
On 2017/05/30 22:10:20, Siggi Cherem (dart-lang) wrote:
> ha - I haven't seen this yet... seems like a sign that we have a misnomer for
> this class... 

Acknowledged.

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/full_emitter/emitter.dart:1269: void
checkEverythingEmitted(Map<ClassEntity, ClassBuilder> classes) {
On 2017/05/30 22:10:20, Siggi Cherem (dart-lang) wrote:
> nit: classes => pendingClassBuilders
>      pendingStatics => pendingClasses
> ?

Done.

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
File pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart (right):

https://codereview.chromium.org/2908153003/diff/1/pkg/compiler/lib/src/js_emi...
pkg/compiler/lib/src/js_emitter/parameter_stub_generator.dart:230: if
(member.enclosingClass != null && member.enclosingClass.isClosure) {
On 2017/05/30 22:10:20, Siggi Cherem (dart-lang) wrote:
> I just noticed that we added `isClosure` to classEntity - do we want to move
> away from that in the long run?)

If anything, it belongs in the J-model.

[Johnni Winther, 2017-05-31 08:24:54]

Description was changed from

==========
It's alive!

With this change the empty program compiles and runs from .dill
==========

to

==========
It's alive!

With this change the empty program compiles and runs from .dill

R=sigmund@google.com

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

[Johnni Winther, 2017-05-31 08:24:55]

Committed patchset #2 (id:20001) manually as
0d0905cd81ea36aac8d6be5037163a9cb8273367 (presubmit successful).