Add an incremental reloader example and a utility tool to trigger a reload by

Add an incremental reloader example and a utility tool to trigger a reload by
hand.

The example includes:
 - an interactive UI that lets you trigger reloads without typing commands
 - a wrapper of the incremental kernel generator that invalidates files based on
 modification time stamps


BUG=
R=scheglov@google.com

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

[Siggi Cherem (dart-lang), 2017-06-06 23:49:45]

sigmund@google.com changed reviewers:
+ paulberry@google.com, scheglov@google.com

[Siggi Cherem (dart-lang), 2017-06-06 23:49:46]

Paul/Konstantin - think of the example folder as what we need to do in
flutter_tools. Does that seem like an appropriate way to wrap the
IncrementalKernelGenerator?

[scheglov, 2017-06-07 01:04:42]

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/example/incre...
File pkg/front_end/example/incremental_reload/compiler_with_invalidation.dart
(right):

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/example/incre...
pkg/front_end/example/incremental_reload/compiler_with_invalidation.dart:37: ?
new FileByteStore('/tmp/ikg_cache')
This will not work on Windows.

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/example/incre...
pkg/front_end/example/incremental_reload/compiler_with_invalidation.dart:78:
lastModified[uri] = new File.fromUri(uri).lastModifiedSync();
??=

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

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:243: Iterable<Uri> get
trackedFiles => _fileUriToFile.keys;
We don't say "tracked" about every method in this class.
I think "get fileUris" would be better name.

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:146:
_invalidatedFiles.addAll(_fsState.trackedFiles);
Please add a test.

[Siggi Cherem (dart-lang), 2017-06-07 20:17:12]

PTAL

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/example/incre...
File pkg/front_end/example/incremental_reload/compiler_with_invalidation.dart
(right):

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/example/incre...
pkg/front_end/example/incremental_reload/compiler_with_invalidation.dart:37: ?
new FileByteStore('/tmp/ikg_cache')
On 2017/06/07 01:04:42, scheglov wrote:
> This will not work on Windows.

Done.

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/example/incre...
pkg/front_end/example/incremental_reload/compiler_with_invalidation.dart:78:
lastModified[uri] = new File.fromUri(uri).lastModifiedSync();
On 2017/06/07 01:04:42, scheglov wrote:
> ??=

Done.

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

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental/file_state.dart:243: Iterable<Uri> get
trackedFiles => _fileUriToFile.keys;
On 2017/06/07 01:04:42, scheglov wrote:
> We don't say "tracked" about every method in this class.
> I think "get fileUris" would be better name.

Done.

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/lib/src/incre...
File pkg/front_end/lib/src/incremental_kernel_generator_impl.dart (right):

https://codereview.chromium.org/2928483005/diff/1/pkg/front_end/lib/src/incre...
pkg/front_end/lib/src/incremental_kernel_generator_impl.dart:146:
_invalidatedFiles.addAll(_fsState.trackedFiles);
On 2017/06/07 01:04:42, scheglov wrote:
> Please add a test.

Done.

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

https://codereview.chromium.org/2928483005/diff/20001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:672:
expect(_getLibraryText(_getLibrary(program, aUri)), contains("int a ="));
Let me know if you prefer that I check the full text of the library. I felt that
this was the main relevant subset to compare before and after.

Aside -- I find it a bit hard to navigate code when we have multiline strings
inlined, especially when those strings start from the margin and are not
indented. Would you be OK if we indent them? It is easy to do for the input
programs, but we'd need to trim the whitespace to do it also for the output
expectations. Another alternative would be to move code snippets to the bottom
of the file and add a bunch of top-level variables/getters for those.

[scheglov, 2017-06-07 20:26:32]

LGTM

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

https://codereview.chromium.org/2928483005/diff/20001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:672:
expect(_getLibraryText(_getLibrary(program, aUri)), contains("int a ="));
On 2017/06/07 20:17:11, Siggi Cherem (dart-lang) wrote:
> Let me know if you prefer that I check the full text of the library. I felt
that
> this was the main relevant subset to compare before and after.
> 
> Aside -- I find it a bit hard to navigate code when we have multiline strings
> inlined, especially when those strings start from the margin and are not
> indented. Would you be OK if we indent them? It is easy to do for the input
> programs, but we'd need to trim the whitespace to do it also for the output
> expectations. Another alternative would be to move code snippets to the bottom
> of the file and add a bunch of top-level variables/getters for those.

Verifying only relevant parts is perfectly fine.

Multiline strings work fine when you use IDEA with its semantic highlighting.
All tests in Analyzer and Analysis Server are written in this style.

I would prefer to keep our test methods self contained, without external
variables.

[Siggi Cherem (dart-lang), 2017-06-07 20:45:02]

Description was changed from

==========
Add an incremental reloader example and a utility tool to trigger a reload by
hand.

The example includes:
 - an interactive UI that lets you trigger reloads without typing commands
 - a wrapper of the incremental kernel generator that invalidates files based on
 modification time stamps


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

to

==========
Add an incremental reloader example and a utility tool to trigger a reload by
hand.

The example includes:
 - an interactive UI that lets you trigger reloads without typing commands
 - a wrapper of the incremental kernel generator that invalidates files based on
 modification time stamps


BUG=
R=scheglov@google.com

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

[Siggi Cherem (dart-lang), 2017-06-07 20:45:03]

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

[Siggi Cherem (dart-lang), 2017-06-07 20:51:09]

thanks

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

https://codereview.chromium.org/2928483005/diff/20001/pkg/front_end/test/incr...
pkg/front_end/test/incremental_kernel_generator_test.dart:672:
expect(_getLibraryText(_getLibrary(program, aUri)), contains("int a ="));
On 2017/06/07 20:26:32, scheglov wrote:
> On 2017/06/07 20:17:11, Siggi Cherem (dart-lang) wrote:
> > Let me know if you prefer that I check the full text of the library. I felt
> that
> > this was the main relevant subset to compare before and after.
> > 
> > Aside -- I find it a bit hard to navigate code when we have multiline
strings
> > inlined, especially when those strings start from the margin and are not
> > indented. Would you be OK if we indent them? It is easy to do for the input
> > programs, but we'd need to trim the whitespace to do it also for the output
> > expectations. Another alternative would be to move code snippets to the
bottom
> > of the file and add a bunch of top-level variables/getters for those.
> 
> Verifying only relevant parts is perfectly fine.
> 
> Multiline strings work fine when you use IDEA with its semantic highlighting.
> All tests in Analyzer and Analysis Server are written in this style.
> 

I have highlighting too :) - the problem is that I do like scanning code
visually and indentation helps with that. The multi-line strings break that flow
for me.

> I would prefer to keep our test methods self contained, without external
> variables.

I can relate to that. The other idea of indenting is a small change. Basically
instead of:

  test_compile_export_cycle() async {
    Uri cUri = writeFile(
        cPath,
        r'''
import 'b.dart';
A a;
B b;
''');

    expect(
        _getLibraryText(library),
        r'''
library;
import self as self;
import "./a.dart" as a;
import "./b.dart" as b;

static field a::A a;
static field b::B b;
''');
  }


We would write it as:

  test_compile_export_cycle() async {
    Uri cUri = writeFile(
        cPath,
        r'''
        import 'b.dart';
        A a;
        B b;
        ''');

    expect(
        _getLibraryText(library),
        r'''
        library;
        import self as self;
        import "./a.dart" as a;
        import "./b.dart" as b;

        static field a::A a;
        static field b::B b;
        '''.replaceAll('\n          ','\n'));
  }



(or we could compare ignoring whitespace, or automate the process of trimming
indentation based on the whitespace on the first line)

Thoughts?

[scheglov, 2017-06-07 21:00:46]

On 2017/06/07 20:51:09, Siggi Cherem (dart-lang) wrote:
> thanks
> 
>
https://codereview.chromium.org/2928483005/diff/20001/pkg/front_end/test/incr...
> File pkg/front_end/test/incremental_kernel_generator_test.dart (right):
> 
>
https://codereview.chromium.org/2928483005/diff/20001/pkg/front_end/test/incr...
> pkg/front_end/test/incremental_kernel_generator_test.dart:672:
> expect(_getLibraryText(_getLibrary(program, aUri)), contains("int a ="));
> On 2017/06/07 20:26:32, scheglov wrote:
> > On 2017/06/07 20:17:11, Siggi Cherem (dart-lang) wrote:
> > > Let me know if you prefer that I check the full text of the library. I
felt
> > that
> > > this was the main relevant subset to compare before and after.
> > > 
> > > Aside -- I find it a bit hard to navigate code when we have multiline
> strings
> > > inlined, especially when those strings start from the margin and are not
> > > indented. Would you be OK if we indent them? It is easy to do for the
input
> > > programs, but we'd need to trim the whitespace to do it also for the
output
> > > expectations. Another alternative would be to move code snippets to the
> bottom
> > > of the file and add a bunch of top-level variables/getters for those.
> > 
> > Verifying only relevant parts is perfectly fine.
> > 
> > Multiline strings work fine when you use IDEA with its semantic
highlighting.
> > All tests in Analyzer and Analysis Server are written in this style.
> > 
> 
> I have highlighting too :) - the problem is that I do like scanning code
> visually and indentation helps with that. The multi-line strings break that
flow
> for me.
> 
> > I would prefer to keep our test methods self contained, without external
> > variables.
> 
> I can relate to that. The other idea of indenting is a small change. Basically
> instead of:
> 
>   test_compile_export_cycle() async {
>     Uri cUri = writeFile(
>         cPath,
>         r'''
> import 'b.dart';
> A a;
> B b;
> ''');
> 
>     expect(
>         _getLibraryText(library),
>         r'''
> library;
> import self as self;
> import "./a.dart" as a;
> import "./b.dart" as b;
> 
> static field a::A a;
> static field b::B b;
> ''');
>   }
> 
> 
> We would write it as:
> 
>   test_compile_export_cycle() async {
>     Uri cUri = writeFile(
>         cPath,
>         r'''
>         import 'b.dart';
>         A a;
>         B b;
>         ''');
> 
>     expect(
>         _getLibraryText(library),
>         r'''
>         library;
>         import self as self;
>         import "./a.dart" as a;
>         import "./b.dart" as b;
> 
>         static field a::A a;
>         static field b::B b;
>         '''.replaceAll('\n          ','\n'));
>   }
> 
> 
> 
> (or we could compare ignoring whitespace, or automate the process of trimming
> indentation based on the whitespace on the first line)
> 
> Thoughts?

I think it's just a matter of getting used to this code style.
The version with indentation breaks my flow.