Implement canonical name scheme in kernel.

Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a Reference object, which is
linked to both the AST node and its CanonicalName, so either can be
created first.

dartk now accepts multiple input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

BUG=
R=ahe@google.com, kmillikin@google.com, kustermann@google.com, sigmund@google.com

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

[asgerf, 2017-01-30 16:50:01]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be linked in instead of being
compiled separately.

dartk still stuffers from incredibly slow cold start due to the
analyzer reading the dart sdk from scratch, so this does not speed
up any builds.

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

dartk still stuffers from incredibly slow cold start due to the
analyzer reading the dart sdk from scratch, so this does not speed
up any builds.

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

[asgerf, 2017-01-30 16:50:20]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

dartk still stuffers from incredibly slow cold start due to the
analyzer reading the dart sdk from scratch, so this does not speed
up any builds.

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

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

[asgerf, 2017-01-30 16:51:29]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

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

[asgerf, 2017-01-30 16:52:58]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill -o program.dill

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

[asgerf, 2017-01-30 16:53:14]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill -o program.dill

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill -l -o program.dill

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

[asgerf, 2017-01-30 16:53:26]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill -l -o program.dill

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

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

[asgerf, 2017-01-30 16:54:41]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

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

[asgerf, 2017-01-30 17:00:05]

asgerf@google.com changed reviewers:
+ ahe@google.com, kmillikin@google.com, kustermann@google.com,
vegorov@google.com

[asgerf, 2017-01-30 17:00:06]

Note that I will not land this until fasta has already landed. I will then
resolve any conflicts as a patch set in this CL.

[ahe, 2017-01-30 17:06:59]

Siggi should be able to help find a reviewer of the changes to rasta.

[ahe, 2017-01-30 17:16:16]

I took a quick look at the changes to ast.dart, and this looks pretty neat.
Here's a few preliminary comments, I'll take a closer look tomorrow.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:224:
getCanonicalNameOfLibrary(this).getChildFromMember(member).linkTo(member);
Would it make sense to move this to "set parent"?

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
File pkg/kernel/lib/canonical_name.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
pkg/kernel/lib/canonical_name.dart:110: void linkTo(LinkedNode target) {
Call this bind?

[Siggi Cherem (dart-lang), 2017-01-30 20:31:05]

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

[Siggi Cherem (dart-lang), 2017-01-30 20:31:06]

rasta change lgtm

[kustermann, 2017-02-01 13:00:55]

Looks pretty nice. First comments from me.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:139: }
Do the replaceWith/replaceChild/remove/parent= methods keep the invariant of the
canonical name tree (i.e. moving nodes around changes their canonical names)?

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:423: }
Should we do some verification in the else branch, that
member.canonicalName.parent.parent is this.canonicalName ?

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:965: Field get field => fieldName?.definition as Field;
Could you use fieldName?.asField here?

Or is this one of the cases where the fieldName is not bound?

(I would assume a FieldInitializer must always be bound to the field after it's
setup)

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:1007: Constructor get target => targetName?.definition
as Constructor;
ditto

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:1045: Constructor get target => targetName?.definition
as Constructor;
ditto

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3218: factory Name.byReference(String name,
CanonicalName library) {
library -> libraryName

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3832: CanonicalName getCanonicalNameOfLibrary(Library
library) {
for consistency you can add the comment here as well

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/binary/ast_t...
File pkg/kernel/lib/binary/ast_to_binary.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/binary/ast_t...
pkg/kernel/lib/binary/ast_to_binary.dart:288: writeName(node.name ??
_emptyName);
Theoretically all of the information for node.name should be present in the
CanonicalName we write out, right?

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
File pkg/kernel/lib/canonical_name.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
pkg/kernel/lib/canonical_name.dart:20: ///         URI of enclosing library
Isn't this the canonical name of the enclosing library?

  236   void addClass(Class class_) {
  237     if (class_.canonicalName == null) {
  238      
getCanonicalNameOfLibrary(this).getChild(class_.name).linkTo(class_);
  239     }

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
pkg/kernel/lib/canonical_name.dart:126: definition = null;
Should this unlink() not also update the _children map?

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/verifier.dart
File pkg/kernel/lib/verifier.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/verifier.dar...
pkg/kernel/lib/verifier.dart:136: void checkCanonicalName(CanonicalName name,
LinkedNode definition) {
You could also verify that name._children is minimal?

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/verifier.dar...
pkg/kernel/lib/verifier.dart:141: throw '$definition has name $name, but that is
bound to ${name.definition}';
maybe run dartfmt

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/test/ast_membenc...
File pkg/kernel/test/ast_membench.sh (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/test/ast_membenc...
pkg/kernel/test/ast_membench.sh:1: testdir=$(dirname $0)
add

#!/bin/sh

and chmod +x ?

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/test/ast_membenc...
pkg/kernel/test/ast_membench.sh:15: echo
remove first_output/second_output again

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel.h#newcode365
runtime/vm/kernel.h:365: class Procedure;
sort

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:321: MallocGrowableArray<Ref<CanonicalName> >
canonical_names_;
This will leak the memory forever! Similar to the strings, you have to make a
table in the [Program] object.

You should be able to use a List<CanonicalName> and then implement
CanonicalName::ReadFrom(Reader* reader) method.

The [Reader] can give you access to the [Program] object (and with that, to the
root canonical name, which is a Child<CanonicalName> of Program and is therefore
correctly freed)

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:665: if (tag == kClass) {
Theoretically we can get rid of this byte  in the binary format now that there
is only one kind of class.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:673: }
Remove the else branch and ASSERT(tag == kClass);

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:779: canonical_name->LinkTo(this);
You could add another method to Reader:

CanonicalName* ReadCanonicalNameReference();
CanonicalName* ReadDefiningCanonicalNameReference(LinkedNode* node_to_link) {
  CanonicalName* name = ReadCanonicalReference();
  ASSERT(name != NULL);
  name->LinkTo(node_to_link);
  return name;
}

and get rid of the ASSERT/LinkTo on all call sites.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:887: init->target_reference_ =
Reference::ReadMemberFrom(reader, false);
If you explicitly pass 'false' everywhere, then get rid of the default value in
the header file.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:1838: 
As mentioned above, this should should be implemented differently, more like:

program->canonical_names_.ReadFrom(reader);

with  

class Program {
List<CanonicalName> canonical_names_;
}

and 

CanonicalName::ReadFrom

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:1847: for (int i = 0; i < canonical_names; ++i) {
Can we guarantee this invariant in the kernel verifier and move this  into a 

#ifdef DEBUG

#endif

block?

[asgerf, 2017-02-02 12:30:28]

Thanks for the reviews so far

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:139: }
On 2017/02/01 13:00:54, kustermann wrote:
> Do the replaceWith/replaceChild/remove/parent= methods keep the invariant of
the
> canonical name tree (i.e. moving nodes around changes their canonical names)?

No.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:224:
getCanonicalNameOfLibrary(this).getChildFromMember(member).linkTo(member);
On 2017/01/30 17:16:16, ahe wrote:
> Would it make sense to move this to "set parent"?

I don't see why. Set parent is the low-level accessor, and addMember is the
convenience method built on top.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:423: }
On 2017/02/01 13:00:54, kustermann wrote:
> Should we do some verification in the else branch, that
> member.canonicalName.parent.parent is this.canonicalName ?

The assertion is equivalent to always doing linkTo, i.e. it checks that it is
not already bound to something else. I would like to always do that, but the
check needed to support the hack in dart2js/rasta, where members are built
before their enclosing classes/libraries. In this case, the member will have a
dummy canonical name.

I've updated the check a bit so the assertion in linkTo will run except in
rasta.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:965: Field get field => fieldName?.definition as Field;
On 2017/02/01 13:00:54, kustermann wrote:
> Could you use fieldName?.asField here?
> 
> Or is this one of the cases where the fieldName is not bound?
> 
> (I would assume a FieldInitializer must always be bound to the field after
it's
> setup) 

Yep, it was supposed to say fieldName?.asField.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:1007: Constructor get target => targetName?.definition
as Constructor;
On 2017/02/01 13:00:54, kustermann wrote:
> ditto

Done.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:1045: Constructor get target => targetName?.definition
as Constructor;
On 2017/02/01 13:00:54, kustermann wrote:
> ditto

Done.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3218: factory Name.byReference(String name,
CanonicalName library) {
On 2017/02/01 13:00:54, kustermann wrote:
> library -> libraryName

Done.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/ast.dart#new...
pkg/kernel/lib/ast.dart:3832: CanonicalName getCanonicalNameOfLibrary(Library
library) {
On 2017/02/01 13:00:54, kustermann wrote:
> for consistency you can add the comment here as well

Done.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/binary/ast_t...
File pkg/kernel/lib/binary/ast_to_binary.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/binary/ast_t...
pkg/kernel/lib/binary/ast_to_binary.dart:288: writeName(node.name ??
_emptyName);
On 2017/02/01 13:00:54, kustermann wrote:
> Theoretically all of the information for node.name should be present in the
> CanonicalName we write out, right?

Yes. I'm not sure the inverse operation is worth implementing though.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
File pkg/kernel/lib/canonical_name.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
pkg/kernel/lib/canonical_name.dart:20: ///         URI of enclosing library
On 2017/02/01 13:00:54, kustermann wrote:
> Isn't this the canonical name of the enclosing library?
> 
>   236   void addClass(Class class_) {
>   237     if (class_.canonicalName == null) {
>   238      
> getCanonicalNameOfLibrary(this).getChild(class_.name).linkTo(class_);
>   239     }

They are the same thing. I thought it would be clearer to write it like this.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
pkg/kernel/lib/canonical_name.dart:110: void linkTo(LinkedNode target) {
On 2017/01/30 17:16:16, ahe wrote:
> Call this bind?

Changed to bindTo

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
pkg/kernel/lib/canonical_name.dart:126: definition = null;
On 2017/02/01 13:00:54, kustermann wrote:
> Should this unlink() not also update the _children map?

'unlink' is not meant to "destroy" the name. It should just remove the two-way
binding.

It's not that simple to garbage collect unused canonical names from the tree, as
they might be intended to be used again. For example, it was designed so this
should work (not that we're actually using it):

  canonicalName..unlink()..linkTo(newDefinition)

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/verifier.dart
File pkg/kernel/lib/verifier.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/verifier.dar...
pkg/kernel/lib/verifier.dart:136: void checkCanonicalName(CanonicalName name,
LinkedNode definition) {
On 2017/02/01 13:00:54, kustermann wrote:
> You could also verify that name._children is minimal?

Follow-up on the previous comment:

It is hard to purge unused canonical names while maintaining a nice API,
especially since we have intermediate canonical name nodes that are not bound to
anything, such as <class>::@methods.  It's not very nice that calling
getChild('@methods') makes the tree invalid if there happens to be no methods.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/verifier.dar...
pkg/kernel/lib/verifier.dart:141: throw '$definition has name $name, but that is
bound to ${name.definition}';
On 2017/02/01 13:00:54, kustermann wrote:
> maybe run dartfmt

Done.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/test/ast_membenc...
File pkg/kernel/test/ast_membench.sh (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/test/ast_membenc...
pkg/kernel/test/ast_membench.sh:1: testdir=$(dirname $0)
On 2017/02/01 13:00:55, kustermann wrote:
> add
> 
> #!/bin/sh
> 
> and chmod +x ?

Done.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/test/ast_membenc...
pkg/kernel/test/ast_membench.sh:15: echo
On 2017/02/01 13:00:55, kustermann wrote:
> remove first_output/second_output again

Done.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel.h#newcode365
runtime/vm/kernel.h:365: class Procedure;
On 2017/02/01 13:00:55, kustermann wrote:
> sort

Done.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:321: MallocGrowableArray<Ref<CanonicalName> >
canonical_names_;
On 2017/02/01 13:00:55, kustermann wrote:
> This will leak the memory forever! Similar to the strings, you have to make a
> table in the [Program] object.
> 
> You should be able to use a List<CanonicalName> and then implement
> CanonicalName::ReadFrom(Reader* reader) method.
> 
> The [Reader] can give you access to the [Program] object (and with that, to
the
> root canonical name, which is a Child<CanonicalName> of Program and is
therefore
> correctly freed)

It is here to support linking in the VM by reading concatenated binaries. I did
not implement that yet, as it is not yet useful as long as we rely on
whole-program transformations, but Kevin is looking into it.

When reading from different binaries, the mapping from index to CanonicalName is
different in each binary, so I thought it best to consider it a mapping that
only exists in a certain scope.

The same problem will come up for strings, since each binary has a different
string table.

As for the memory leak: my intention was that CanonicalName destroys its
children, i.e. the Program destroys the root name, and the root name recursively
destroys everything else. I forgot to actually put the list of children on the
CanonicalName, though, but I think it should be fixed now (thanks for pointing
it out!).

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:665: if (tag == kClass) {
On 2017/02/01 13:00:55, kustermann wrote:
> Theoretically we can get rid of this byte  in the binary format now that there
> is only one kind of class.

But the redundancy does make debugging a bit nicer, so I don't mind having it.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:673: }
On 2017/02/01 13:00:55, kustermann wrote:
> Remove the else branch and ASSERT(tag == kClass);

Done.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:779: canonical_name->LinkTo(this);
On 2017/02/01 13:00:55, kustermann wrote:
> You could add another method to Reader:
> 
> CanonicalName* ReadCanonicalNameReference();
> CanonicalName* ReadDefiningCanonicalNameReference(LinkedNode* node_to_link) {
>   CanonicalName* name = ReadCanonicalReference();
>   ASSERT(name != NULL);
>   name->LinkTo(node_to_link);
>   return name;
> }
> 
> and get rid of the ASSERT/LinkTo on all call sites.

Done.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:887: init->target_reference_ =
Reference::ReadMemberFrom(reader, false);
On 2017/02/01 13:00:55, kustermann wrote:
> If you explicitly pass 'false' everywhere, then get rid of the default value
in
> the header file.

No, it was just here. I've removed the redundant argument.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:1838: 
On 2017/02/01 13:00:55, kustermann wrote:
> As mentioned above, this should should be implemented differently, more like:
> 
> program->canonical_names_.ReadFrom(reader);
> 
> with  
> 
> class Program {
> List<CanonicalName> canonical_names_;
> }
> 
> and 
> 
> CanonicalName::ReadFrom

From previous answer: it won't work for linking in the VM.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:1847: for (int i = 0; i < canonical_names; ++i) {
On 2017/02/01 13:00:55, kustermann wrote:
> Can we guarantee this invariant in the kernel verifier and move this  into a 
> 
> #ifdef DEBUG
> 
> #endif
> 
> block?

Done.

[ahe, 2017-02-02 16:24:02]

I haven't reviewed the C++ code, and I still haven't reviewed
pkg/kernel/lib/binary/ast_from_binary.dart.

Expect thumbs up from me when I'm done with ast_from_binary.

If you like, I can help you update fasta tomorrow.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/bin/dartk.dart
File pkg/kernel/bin/dartk.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/bin/dartk.da...
pkg/kernel/bin/dartk.dart:344: Uri fileUri = new Uri(scheme: 'file', path:
file);
Uri.base.resolve(file) should be able to replace the above two lines.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:224: // This should not run from the dart2js/rasta
adaptor.
Add TODO?

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/canonica...
File pkg/kernel/lib/canonical_name.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/canonica...
pkg/kernel/lib/canonical_name.dart:74: /// Used as temporary measure to get the
dart2js-kernel adaptor working.
Add TODO to remove?

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/canonica...
pkg/kernel/lib/canonical_name.dart:84: /// True if this is a fake canonical name
object used by the dart2js/kernel
Ditto?

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/canonica...
pkg/kernel/lib/canonical_name.dart:135: Library get asLibrary {
Nice!

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/reposito...
File pkg/kernel/lib/repository.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/reposito...
pkg/kernel/lib/repository.dart:12: class Repository {
Can you remove this class now?

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures/kernel_chain.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures/kernel_chain.dart:1: // Copyright (c) 2016, the Dart
project authors. Please see the AUTHORS file
FYI: I'll take care of unifying this with the copy in fasta.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/test/verify_...
File pkg/kernel/test/verify_test.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/test/verify_...
pkg/kernel/test/verify_test.dart:257: Program program;
It always confuses me when fields aren't first in a class.

[asgerf, 2017-02-03 10:12:16]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts multiple input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

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

[asgerf, 2017-02-03 10:31:16]

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/bin/dartk.dart
File pkg/kernel/bin/dartk.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/bin/dartk.da...
pkg/kernel/bin/dartk.dart:344: Uri fileUri = new Uri(scheme: 'file', path:
file);
On 2017/02/02 16:24:01, ahe wrote:
> Uri.base.resolve(file) should be able to replace the above two lines.

Done.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:224: // This should not run from the dart2js/rasta
adaptor.
On 2017/02/02 16:24:01, ahe wrote:
> Add TODO?

I don't think it actually helps to spray non-actionable TODOs in the codebase.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/canonica...
File pkg/kernel/lib/canonical_name.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/canonica...
pkg/kernel/lib/canonical_name.dart:74: /// Used as temporary measure to get the
dart2js-kernel adaptor working.
On 2017/02/02 16:24:01, ahe wrote:
> Add TODO to remove?

Same as previous answer.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/canonica...
pkg/kernel/lib/canonical_name.dart:84: /// True if this is a fake canonical name
object used by the dart2js/kernel
On 2017/02/02 16:24:01, ahe wrote:
> Ditto?

Same as previous answer.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/canonica...
pkg/kernel/lib/canonical_name.dart:135: Library get asLibrary {
On 2017/02/02 16:24:01, ahe wrote:
> Nice!

Acknowledged.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/reposito...
File pkg/kernel/lib/repository.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/reposito...
pkg/kernel/lib/repository.dart:12: class Repository {
On 2017/02/02 16:24:01, ahe wrote:
> Can you remove this class now?

Yes, thank you. I actually meant to remove it.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/test/closure...
File pkg/kernel/test/closures/kernel_chain.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/test/closure...
pkg/kernel/test/closures/kernel_chain.dart:1: // Copyright (c) 2016, the Dart
project authors. Please see the AUTHORS file
On 2017/02/02 16:24:01, ahe wrote:
> FYI: I'll take care of unifying this with the copy in fasta.

Thanks

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/test/verify_...
File pkg/kernel/test/verify_test.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/test/verify_...
pkg/kernel/test/verify_test.dart:257: Program program;
On 2017/02/02 16:24:01, ahe wrote:
> It always confuses me when fields aren't first in a class.

Fixed.

[kustermann, 2017-02-03 11:48:20]

LGTM

One more comment: Since not everybody will be that familiar with the invariants
of the IR and canonical names add more invariant it would be very beneficial to
describe the APIs which authors of transformations should use (maybe add the
comment to the Transformer class). 

More specifically:
* low level apis: 'parent =', ... 
* higher level apis: 'addMember()' ...

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
File pkg/kernel/lib/canonical_name.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
pkg/kernel/lib/canonical_name.dart:20: ///         URI of enclosing library
On 2017/02/02 12:30:27, asgerf wrote:
> On 2017/02/01 13:00:54, kustermann wrote:
> > Isn't this the canonical name of the enclosing library?
> > 
> >   236   void addClass(Class class_) {
> >   237     if (class_.canonicalName == null) {
> >   238      
> > getCanonicalNameOfLibrary(this).getChild(class_.name).linkTo(class_);
> >   239     }
> 
> They are the same thing. I thought it would be clearer to write it like this.

Up to you, but for all the other things, like Field, you describe it as
"Canonical name of enclosing ... or library", so I'd do it here as well.  
 
i.e it's not an URI but a canonical name of the library.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:321: MallocGrowableArray<Ref<CanonicalName> >
canonical_names_;
On 2017/02/02 12:30:28, asgerf wrote:
> On 2017/02/01 13:00:55, kustermann wrote:
> > This will leak the memory forever! Similar to the strings, you have to make
a
> > table in the [Program] object.
> > 
> > You should be able to use a List<CanonicalName> and then implement
> > CanonicalName::ReadFrom(Reader* reader) method.
> > 
> > The [Reader] can give you access to the [Program] object (and with that, to
> the
> > root canonical name, which is a Child<CanonicalName> of Program and is
> therefore
> > correctly freed)
> 
> It is here to support linking in the VM by reading concatenated binaries. I
did
> not implement that yet, as it is not yet useful as long as we rely on
> whole-program transformations, but Kevin is looking into it.
> 
> When reading from different binaries, the mapping from index to CanonicalName
is
> different in each binary, so I thought it best to consider it a mapping that
> only exists in a certain scope.
> 
> The same problem will come up for strings, since each binary has a different
> string table.
> 
> As for the memory leak: my intention was that CanonicalName destroys its
> children, i.e. the Program destroys the root name, and the root name
recursively
> destroys everything else. I forgot to actually put the list of children on the
> CanonicalName, though, but I think it should be fixed now (thanks for pointing
> it out!).

Normally I'd argue that we should do one thing at a time and be consistent (in
this case, treat strings/canonical-names the same). But leave it as it is now.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:389: ~CanonicalName();
nit: Destructors go first in the public: section, methods come afterwards.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:661: canonical_name->BindTo(this);
Use reader->ReadDefiningCanonicalNameReference(this)

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:688: canonical_name->BindTo(this);
Use reader->ReadDefiningCanonicalNameReference(this)

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:1816: for (int i = 0; i < canonical_names; ++i) {
nit: vm convention is to use intptr_t instead of int (in almost all places)

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:1823: }
You can 

reader->helper()->SetCanonicalNameCount(1 + canonical_names);
reader->helper()->SetCanonicalName(0, program->canonical_name_root_);

before the loop and get rid of the if/else and -1 special cases.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:1840: for (int i = 0; i < canonical_names; ++i) {
nit: vm convention is to use intptr_t instead of int (in almost all places)

[kustermann, 2017-02-03 11:50:58]

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:399: };
like with other classes, you should probably add a

DISALLOW_COPY_AND_ASSIGN(CanonicalName);

here

[asgerf, 2017-02-03 12:47:38]

Thanks for the reviews

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
File pkg/kernel/lib/canonical_name.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/canonical_na...
pkg/kernel/lib/canonical_name.dart:20: ///         URI of enclosing library
On 2017/02/03 11:48:20, kustermann wrote:
> On 2017/02/02 12:30:27, asgerf wrote:
> > On 2017/02/01 13:00:54, kustermann wrote:
> > > Isn't this the canonical name of the enclosing library?
> > > 
> > >   236   void addClass(Class class_) {
> > >   237     if (class_.canonicalName == null) {
> > >   238      
> > > getCanonicalNameOfLibrary(this).getChild(class_.name).linkTo(class_);
> > >   239     }
> > 
> > They are the same thing. I thought it would be clearer to write it like
this.
> 
> Up to you, but for all the other things, like Field, you describe it as
> "Canonical name of enclosing ... or library", so I'd do it here as well.  
>  
> i.e it's not an URI but a canonical name of the library.

Done.

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2665723002/diff/1/runtime/vm/kernel_binary.cc...
runtime/vm/kernel_binary.cc:321: MallocGrowableArray<Ref<CanonicalName> >
canonical_names_;
On 2017/02/03 11:48:20, kustermann wrote:
> On 2017/02/02 12:30:28, asgerf wrote:
> > On 2017/02/01 13:00:55, kustermann wrote:
> > > This will leak the memory forever! Similar to the strings, you have to
make
> a
> > > table in the [Program] object.
> > > 
> > > You should be able to use a List<CanonicalName> and then implement
> > > CanonicalName::ReadFrom(Reader* reader) method.
> > > 
> > > The [Reader] can give you access to the [Program] object (and with that,
to
> > the
> > > root canonical name, which is a Child<CanonicalName> of Program and is
> > therefore
> > > correctly freed)
> > 
> > It is here to support linking in the VM by reading concatenated binaries. I
> did
> > not implement that yet, as it is not yet useful as long as we rely on
> > whole-program transformations, but Kevin is looking into it.
> > 
> > When reading from different binaries, the mapping from index to
CanonicalName
> is
> > different in each binary, so I thought it best to consider it a mapping that
> > only exists in a certain scope.
> > 
> > The same problem will come up for strings, since each binary has a different
> > string table.
> > 
> > As for the memory leak: my intention was that CanonicalName destroys its
> > children, i.e. the Program destroys the root name, and the root name
> recursively
> > destroys everything else. I forgot to actually put the list of children on
the
> > CanonicalName, though, but I think it should be fixed now (thanks for
pointing
> > it out!).
> 
> Normally I'd argue that we should do one thing at a time and be consistent (in
> this case, treat strings/canonical-names the same). But leave it as it is now.

Acknowledged.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel.h
File runtime/vm/kernel.h (right):

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:389: ~CanonicalName();
On 2017/02/03 11:48:20, kustermann wrote:
> nit: Destructors go first in the public: section, methods come afterwards.

Done.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:399: };
On 2017/02/03 11:50:58, kustermann wrote:
> like with other classes, you should probably add a
> 
> DISALLOW_COPY_AND_ASSIGN(CanonicalName);
> 
> here

Done.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
File runtime/vm/kernel_binary.cc (right):

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:661: canonical_name->BindTo(this);
On 2017/02/03 11:48:20, kustermann wrote:
> Use reader->ReadDefiningCanonicalNameReference(this)

Done.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:688: canonical_name->BindTo(this);
On 2017/02/03 11:48:20, kustermann wrote:
> Use reader->ReadDefiningCanonicalNameReference(this)

Done.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:1816: for (int i = 0; i < canonical_names; ++i) {
On 2017/02/03 11:48:20, kustermann wrote:
> nit: vm convention is to use intptr_t instead of int (in almost all places)

Done.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:1823: }
On 2017/02/03 11:48:20, kustermann wrote:
> You can 
> 
> reader->helper()->SetCanonicalNameCount(1 + canonical_names);
> reader->helper()->SetCanonicalName(0, program->canonical_name_root_);
> 
> before the loop and get rid of the if/else and -1 special cases.

I just tried it, but it didn't really work out that well.

One thing that bothers me is how ReadCanonicalNameReference ends up looking.
Currently it looks like this:

    int index = ReadUInt();
    if (index == 0) return NULL;
    CanonicalName* name = builder_.GetCanonicalName(index - 1);

Afterwards it becomes:

    int index = ReadUInt();
    if (index == 0) return NULL;
    CanonicalName* name = builder_.GetCanonicalName(index);

I don't think it is as self-explanatory (what's up with index 0?) and does not
directly correspond to binary.md anymore.

https://codereview.chromium.org/2665723002/diff/40001/runtime/vm/kernel_binar...
runtime/vm/kernel_binary.cc:1840: for (int i = 0; i < canonical_names; ++i) {
On 2017/02/03 11:48:20, kustermann wrote:
> nit: vm convention is to use intptr_t instead of int (in almost all places)

Done.

[asgerf, 2017-02-03 13:05:04]

BTW about the documentation comment:

I agree that the documentation should be more clear about the
low-level/high-level split.

Kevin, Luke, and I had a conversation today about the API for user-provided
kernel transformers, and I think there is a need for more guidance about what is
the recommended way of doing various tasks, in addition to just documenting what
each method is doing.

[asgerf, 2017-02-03 15:35:37]

Unfortunately this CL will have to sit for another week, as I am on vacation
next week.

[ahe, 2017-02-13 10:07:08]

lgtm

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/binary/ast_f...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/binary/ast_f...
pkg/kernel/lib/binary/ast_from_binary.dart:174: /// implementation, the order is
corrected.
I suggest that you add some documentation of what readObject is supposed to.

I'm particularly curious about how readObject is supposed to deal with existing
objects, for both values of _isReadingLibraryImplementation.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:184: // - ignored if the library
impementation is already in memory
implementation

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:187: // the AST and hence should not
be added again.
I'm not really understanding how this comment relates to the code below. How are
the two cases handled?

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:213: void readMultiFile(Program
program) {
Suggestion: rename this to readProgram and call the other one
readSingleFileProgram.

[asgerf, 2017-02-16 14:16:49]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

The most breaking change in the API is that classes and members
cannot be referenced before they have been assigned a name, and their
name is not assigned until they are added to a class or library.

dartk now accepts multiple input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

dartk now accepts multiple input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

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

[asgerf, 2017-02-16 14:22:42]

Someone PTAL.

I had to change the API to get it working in fasta.
AST nodes can now be referenced before they have a canonical name. Canonical
names are now mostly just an artifact of how serialization works.

There is an extra intermediate object around linked nodes, a LinkedNodeBox, but
the number of dereferences to get from a reference to its definition is the
same.

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/binary/ast_f...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2665723002/diff/1/pkg/kernel/lib/binary/ast_f...
pkg/kernel/lib/binary/ast_from_binary.dart:174: /// implementation, the order is
corrected.
On 2017/02/13 10:07:07, ahe wrote:
> I suggest that you add some documentation of what readObject is supposed to.
> 
> I'm particularly curious about how readObject is supposed to deal with
existing
> objects, for both values of _isReadingLibraryImplementation.

Done.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/binary/a...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:184: // - ignored if the library
impementation is already in memory
On 2017/02/13 10:07:08, ahe wrote:
> implementation

Done.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:187: // the AST and hence should not
be added again.
On 2017/02/13 10:07:08, ahe wrote:
> I'm not really understanding how this comment relates to the code below. How
are
> the two cases handled?

Moved the comment down closer to the check.

https://codereview.chromium.org/2665723002/diff/20001/pkg/kernel/lib/binary/a...
pkg/kernel/lib/binary/ast_from_binary.dart:213: void readMultiFile(Program
program) {
On 2017/02/13 10:07:08, ahe wrote:
> Suggestion: rename this to readProgram and call the other one
> readSingleFileProgram.

Done.

[Kevin Millikin (Google), 2017-02-22 09:09:30]

LGTM though I found LinkedNode and LinkedNodeBox too generic.

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/binary.md#n...
pkg/kernel/binary.md:109: CanonicalNameReference mainMethod;
Is this ProcedureReference, or can it also be a field (so we could say
MemberReference)?

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/binary.md#n...
pkg/kernel/binary.md:244: FileOffset fileOffset;
CanonicalNameReference canonicalName;

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/analyze...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/analyze...
pkg/kernel/lib/analyzer/loader.dart:809: ast.Program loadProgram(Uri
mainLibrary, {Target target, bool compileSdk}) {
It's a little weird that this mutates the loader's program and also returns it. 
I think it makes the call sites slightly misleading.  Recommend making it return
void.

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/ast.dar...
pkg/kernel/lib/ast.dart:162: abstract class LinkedNode extends TreeNode {
Suggestion: LinkedNode ==> NamedNode, LinkedNodeBox ==> Reference.  Then change
Box everywhere to Ref or Reference.

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_from_binary.dart:249: void _readProgram(Program
program) {
_readOneProgram?

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/verifie...
File pkg/kernel/lib/verifier.dart (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/verifie...
pkg/kernel/lib/verifier.dart:175: void checkCanonicalName(CanonicalName name,
LinkedNode definition) {
Get rid of the function?  Or if we sometimes want it, pass a flag to the verify
to tell it to check?

[asgerf, 2017-02-22 10:06:54]

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/binary.md
File pkg/kernel/binary.md (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/binary.md#n...
pkg/kernel/binary.md:109: CanonicalNameReference mainMethod;
On 2017/02/22 09:09:30, Kevin Millikin (Google) wrote:
> Is this ProcedureReference, or can it also be a field (so we could say
> MemberReference)?

It is a ProcedureReference. Fixed.

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/binary.md#n...
pkg/kernel/binary.md:244: FileOffset fileOffset;
On 2017/02/22 09:09:30, Kevin Millikin (Google) wrote:
> CanonicalNameReference canonicalName;

Done.

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/analyze...
File pkg/kernel/lib/analyzer/loader.dart (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/analyze...
pkg/kernel/lib/analyzer/loader.dart:809: ast.Program loadProgram(Uri
mainLibrary, {Target target, bool compileSdk}) {
On 2017/02/22 09:09:30, Kevin Millikin (Google) wrote:
> It's a little weird that this mutates the loader's program and also returns
it. 
> I think it makes the call sites slightly misleading.  Recommend making it
return
> void.

Done.

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/ast.dar...
pkg/kernel/lib/ast.dart:162: abstract class LinkedNode extends TreeNode {
On 2017/02/22 09:09:30, Kevin Millikin (Google) wrote:
> Suggestion: LinkedNode ==> NamedNode, LinkedNodeBox ==> Reference.  Then
change
> Box everywhere to Ref or Reference.

Done.

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_from_binary.dart:249: void _readProgram(Program
program) {
On 2017/02/22 09:09:30, Kevin Millikin (Google) wrote:
> _readOneProgram?

Done.

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/verifie...
File pkg/kernel/lib/verifier.dart (right):

https://codereview.chromium.org/2665723002/diff/120001/pkg/kernel/lib/verifie...
pkg/kernel/lib/verifier.dart:175: void checkCanonicalName(CanonicalName name,
LinkedNode definition) {
On 2017/02/22 09:09:30, Kevin Millikin (Google) wrote:
> Get rid of the function?  Or if we sometimes want it, pass a flag to the
verify
> to tell it to check?

Removed the function. I did not intend to upload the outcommented version.

[asgerf, 2017-02-23 11:42:40]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a canonical name, although
the API mostly makes this transparent.

dartk now accepts multiple input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a Reference object, which is
linked to both the AST node and its CanonicalName, so either can be
created first.

dartk now accepts multiple input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

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

[asgerf, 2017-02-23 13:12:17]

Description was changed from

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a Reference object, which is
linked to both the AST node and its CanonicalName, so either can be
created first.

dartk now accepts multiple input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

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

to

==========
Implement canonical name scheme in kernel.

This adds a class CanonicalName that can represent a library, class,
or member.  All references now go through a Reference object, which is
linked to both the AST node and its CanonicalName, so either can be
created first.

dartk now accepts multiple input files:
- If multiple dart files are given, they are all compiled.
- If multiple binaries are given, they are linked together.
Mixed dart and binary input is not supported by dartk.

dartk now has a flag --include-sdk which includes the entire SDK in
the output.  This is so the SDK can be compiled alone and then linked.

Example of compiling separately and then linking:
  dartk foo.dart -o foo.dill
  dartk main.dart -o main.dill
  dartk --include-sdk -o sdk.dill
  dartk main.dill foo.dill sdk.dill --target=vm --link -o program.dill

dartk still has incredibly slow cold start due to the analyzer loading
the dart sdk, so this does not actually speed things up at the moment.

BUG=
R=ahe@google.com, kmillikin@google.com, kustermann@google.com,
sigmund@google.com

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

[asgerf, 2017-02-23 13:12:19]

Committed patchset #8 (id:140001) manually as
8bfc4b47c0ae5b506e52da7c7cf64499025c693f (presubmit successful).