Add LimitedBinaryPrinter, tests and switch incremental generator to it.

Add LimitedBinaryPrinter, tests and switch incremental generator to it.

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

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

[ahe, 2017-05-19 07:43:06]

ahe@google.com changed reviewers:
+ asgerf@google.com

[ahe, 2017-05-19 07:43:06]

Asger, could you take a look?

[ahe, 2017-05-22 09:27:05]

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart (right):

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart:1: // Copyright (c)
2017, the Dart project authors.  Please see the AUTHORS file
I think this file should be moved to package:kernel.

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart:73: void
_visitReferencedName(CanonicalName name) {
Rename to handleReferencedName or something as this isn't a visit method.

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:319: 
Can you check that b.dart has been removed here?

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

https://codereview.chromium.org/2896493002/diff/1/pkg/kernel/lib/binary/ast_t...
pkg/kernel/lib/binary/ast_to_binary.dart:22: StringIndexer _stringIndexer;
You can keep this final by adding an optional parameter to the constructor.

https://codereview.chromium.org/2896493002/diff/1/pkg/kernel/lib/binary/ast_t...
pkg/kernel/lib/binary/ast_to_binary.dart:22: StringIndexer _stringIndexer;
Seems like you need this field to be public.

[scheglov, 2017-05-22 20:36:01]

PTAL

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart (right):

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart:1: // Copyright (c)
2017, the Dart project authors.  Please see the AUTHORS file
On 2017/05/22 09:27:05, ahe wrote:
> I think this file should be moved to package:kernel.

I would not argue about.
But I'm very cautious about putting anything into package:kernel and causing "it
does not belong here" reaction from its authors.
OTOH, it is as well possible to get the opposite reaction.
So, I decided to be conservative and do as little changes to kernel as possible.
I would be happy to move it if this is OK with package:kernel authors.

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart:73: void
_visitReferencedName(CanonicalName name) {
On 2017/05/22 09:27:05, ahe wrote:
> Rename to handleReferencedName or something as this isn't a visit method.

Done.

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:319: 
On 2017/05/22 09:27:05, ahe wrote:
> Can you check that b.dart has been removed here?

I'm not sure what we will verify by doing this.

That libraries.remove(initialLibrary) worked?
I think this test would belong to List unit test.

That root.removeChild(initialLibrary.importUri.toString()) removed its child?
Again, this is something to test in CanonicalName unit test, which does not
exist, but I'd be happy to add it, if this is what you mean by your comment.

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

https://codereview.chromium.org/2896493002/diff/1/pkg/kernel/lib/binary/ast_t...
pkg/kernel/lib/binary/ast_to_binary.dart:22: StringIndexer _stringIndexer;
On 2017/05/22 09:27:05, ahe wrote:
> You can keep this final by adding an optional parameter to the constructor.

Done.

https://codereview.chromium.org/2896493002/diff/1/pkg/kernel/lib/binary/ast_t...
pkg/kernel/lib/binary/ast_to_binary.dart:22: StringIndexer _stringIndexer;
On 2017/05/22 09:27:05, ahe wrote:
> Seems like you need this field to be public.

Done.

[Siggi Cherem (dart-lang), 2017-05-23 20:54:51]

I don't have much else to add on top of Peter's comments. Only minor suggestions
below. It also seems reasonable to me to put the new class in the kernel
package. Kevin - let us know about that.

I'd wait to hear from Kevin on that, but otherwise lgtm.

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart (right):

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart:21: var
stringIndexer = this.stringIndexer as ReferencesStringIndexer;
nit: I've seen this style in fasta so far to use an assignment like:

ReferencesStringIndexer stringIndexer = this.stringIndexer;

instead of a cast.

It implies an implicit cast in strong-mode, but in spec-mode the cast will only
exist in checked-mode. this wont matter in the long run, but for now it doesn't
hurt.

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/test/incr...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:361:
initialProgram.libraries.add(loadedLibrary);
nit: either rename initialProgram or add a second variable for the program after
you start modifying it.

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:367: // The program
with re-added library passes verification.
I'd delete this and the comment above, I think the code reads similar to this
sentence, especially if we rename `initialProgram` or add a second varaible name
for it (e.g. `modifiedProgram`)

[scheglov, 2017-05-23 21:54:43]

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart (right):

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/limited_ast_to_binary.dart:21: var
stringIndexer = this.stringIndexer as ReferencesStringIndexer;
On 2017/05/23 20:54:51, Siggi Cherem (dart-lang) wrote:
> nit: I've seen this style in fasta so far to use an assignment like:
> 
> ReferencesStringIndexer stringIndexer = this.stringIndexer;
> 
> instead of a cast.
> 
> It implies an implicit cast in strong-mode, but in spec-mode the cast will
only
> exist in checked-mode. this wont matter in the long run, but for now it
doesn't
> hurt.

Done.

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/test/incr...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:361:
initialProgram.libraries.add(loadedLibrary);
On 2017/05/23 20:54:51, Siggi Cherem (dart-lang) wrote:
> nit: either rename initialProgram or add a second variable for the program
after
> you start modifying it.

I renamed it to just "program".

https://codereview.chromium.org/2896493002/diff/20001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:367: // The program
with re-added library passes verification.
On 2017/05/23 20:54:51, Siggi Cherem (dart-lang) wrote:
> I'd delete this and the comment above, I think the code reads similar to this
> sentence, especially if we rename `initialProgram` or add a second varaible
name
> for it (e.g. `modifiedProgram`)

OK, I removed both comments.

A second variable name does not work, we have one instance, it does not seem
useful to have aliases for it.

[ahe, 2017-05-24 08:36:00]

LGTM provided you use a package URI in incremental_kernel_generator_test.dart.

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/test/incremen...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2896493002/diff/1/pkg/front_end/test/incremen...
pkg/front_end/test/incremental_kernel_generator_test.dart:319: 
On 2017/05/22 20:36:01, scheglov wrote:
> On 2017/05/22 09:27:05, ahe wrote:
> > Can you check that b.dart has been removed here?
> 
> I'm not sure what we will verify by doing this.
> 
> That libraries.remove(initialLibrary) worked?
> I think this test would belong to List unit test.
> 
> That root.removeChild(initialLibrary.importUri.toString()) removed its child?
> Again, this is something to test in CanonicalName unit test, which does not
> exist, but I'd be happy to add it, if this is what you mean by your comment.

I guess you're right. I'm just looking for some way to validate that you
actually read the new version of b.dart.

https://codereview.chromium.org/2896493002/diff/40001/pkg/front_end/test/incr...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2896493002/diff/40001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:11: import
'../../kernel/lib/binary/limited_ast_to_binary.dart';
You probably want to use a package URI here.

[Kevin Millikin (Google), 2017-05-24 08:58:58]

LGTM.

[scheglov, 2017-05-24 23:17:36]

Description was changed from

==========
Add LimitedBinaryPrinter, tests and switch incremental generator to it.

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

to

==========
Add LimitedBinaryPrinter, tests and switch incremental generator to it.

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

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

[scheglov, 2017-05-24 23:17:37]

Committed patchset #3 (id:40001) manually as
97d587520cb701a3122bdc72153683e0aa7039c5 (presubmit successful).

[scheglov, 2017-05-25 17:44:09]

https://codereview.chromium.org/2896493002/diff/40001/pkg/front_end/test/incr...
File pkg/front_end/test/incremental_kernel_generator_test.dart (right):

https://codereview.chromium.org/2896493002/diff/40001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:11: import
'../../kernel/lib/binary/limited_ast_to_binary.dart';
On 2017/05/24 08:36:00, ahe wrote:
> You probably want to use a package URI here.

Fixed.
Thanks.