Disable formatting and add new-lines to make tests faster.

Disable formatting and add new-lines to make tests faster.

R=vsm@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/7c9f957ccb2e5d4828898d88ec1c769502190334

[Siggi Cherem (dart-lang), 2015-02-26 21:03:47]

sigmund@google.com changed reviewers:
+ jmesserly@google.com, leafp@google.com

[Siggi Cherem (dart-lang), 2015-02-26 21:03:49]

this makes all_tests.dart run in 10s on my machine.

https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
File lib/src/codegen/dart_codegen.dart (right):

https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
lib/src/codegen/dart_codegen.dart:78: if (x == '{') {
turns out that the ToStringVIsitor class calls print with individual symbols (;,
{, and }), so this is sufficient for the purpose of testing the output of the
core libraries.

[vsm, 2015-02-26 21:09:26]

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

[vsm, 2015-02-26 21:09:27]

lgtm

Somehow, it seems appropriate to have reviewed this CL while waiting for tests
to finish.  :-)

[Siggi Cherem (dart-lang), 2015-02-26 21:14:01]

Committed patchset #3 (id:20001) manually as
7c9f957ccb2e5d4828898d88ec1c769502190334 (presubmit successful).

[Leaf, 2015-02-26 21:42:14]

https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
File lib/src/codegen/dart_codegen.dart (right):

https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
lib/src/codegen/dart_codegen.dart:78: if (x == '{') {
On 2015/02/26 21:03:49, Siggi Cherem (dart-lang) wrote:
> turns out that the ToStringVIsitor class calls print with individual symbols
(;,
> {, and }), so this is sufficient for the purpose of testing the output of the
> core libraries.

Won't this break things?  For example, printing a string interpolation seems to
call print with a single '}' to terminate the interpolation, but we don't want
to add a new line in the middle of the string, right?

https://codereview.chromium.org/963593002/diff/20001/test/dart_codegen/expect...
File test/dart_codegen/expect/_internal/iterable.dart (right):

https://codereview.chromium.org/963593002/diff/20001/test/dart_codegen/expect...
test/dart_codegen/expect/_internal/iterable.dart:128: ";
This is a bad newline break.

[Siggi Cherem (dart-lang), 2015-02-26 21:59:57]

On 2015/02/26 21:42:14, leafp wrote:
>
https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
> File lib/src/codegen/dart_codegen.dart (right):
> 
>
https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
> lib/src/codegen/dart_codegen.dart:78: if (x == '{') {
> On 2015/02/26 21:03:49, Siggi Cherem (dart-lang) wrote:
> > turns out that the ToStringVIsitor class calls print with individual symbols
> (;,
> > {, and }), so this is sufficient for the purpose of testing the output of
the
> > core libraries.
> 
> Won't this break things?  For example, printing a string interpolation seems
to
> call print with a single '}' to terminate the interpolation, but we don't want
> to add a new line in the middle of the string, right?
> 
>
https://codereview.chromium.org/963593002/diff/20001/test/dart_codegen/expect...
> File test/dart_codegen/expect/_internal/iterable.dart (right):
> 
>
https://codereview.chromium.org/963593002/diff/20001/test/dart_codegen/expect...
> test/dart_codegen/expect/_internal/iterable.dart:128: ";
> This is a bad newline break.

good catch!

I'll send a follow up CL to fix it. I could keep just new lines on ;, but I
thought the braces could help with readability a bit.

Alternatively, I can add an exception for interpolated strings. Any other cases
you can think of that we should consider?

[Leaf, 2015-02-26 22:06:37]

On 2015/02/26 21:59:57, Siggi Cherem (dart-lang) wrote:
> On 2015/02/26 21:42:14, leafp wrote:
> >
>
https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
> > File lib/src/codegen/dart_codegen.dart (right):
> > 
> >
>
https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
> > lib/src/codegen/dart_codegen.dart:78: if (x == '{') {
> > On 2015/02/26 21:03:49, Siggi Cherem (dart-lang) wrote:
> > > turns out that the ToStringVIsitor class calls print with individual
symbols
> > (;,
> > > {, and }), so this is sufficient for the purpose of testing the output of
> the
> > > core libraries.
> > 
> > Won't this break things?  For example, printing a string interpolation seems
> to
> > call print with a single '}' to terminate the interpolation, but we don't
want
> > to add a new line in the middle of the string, right?
> > 
> >
>
https://codereview.chromium.org/963593002/diff/20001/test/dart_codegen/expect...
> > File test/dart_codegen/expect/_internal/iterable.dart (right):
> > 
> >
>
https://codereview.chromium.org/963593002/diff/20001/test/dart_codegen/expect...
> > test/dart_codegen/expect/_internal/iterable.dart:128: ";
> > This is a bad newline break.
> 
> good catch!
> 
> I'll send a follow up CL to fix it. I could keep just new lines on ;, but I
> thought the braces could help with readability a bit.
> 
> Alternatively, I can add an exception for interpolated strings. Any other
cases
> you can think of that we should consider?

Not sure - I'm a little nervous about this approach in that it's hard to predict
how someone might be using the printer (e.g. in a worst case, someone could just
iterate through a string of characters in a loop).  

Would it work to just add override of a few methods in UnitGeneratorCommon to
add newlines there?  Maybe adding a new line after every statement, and a few
other places that we're sure are valid?

Alternatively, you could hide this behind another flag - my worry is that for
large apps we might not be able to run with dart-gen-fmt (because of time) but
also not without it (because it hits an edge case).

[Siggi Cherem (dart-lang), 2015-02-26 22:26:11]

On 2015/02/26 22:06:37, leafp wrote:
> On 2015/02/26 21:59:57, Siggi Cherem (dart-lang) wrote:
> > On 2015/02/26 21:42:14, leafp wrote:
> > >
> >
>
https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
> > > File lib/src/codegen/dart_codegen.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
> > > lib/src/codegen/dart_codegen.dart:78: if (x == '{') {
> > > On 2015/02/26 21:03:49, Siggi Cherem (dart-lang) wrote:
> > > > turns out that the ToStringVIsitor class calls print with individual
> symbols
> > > (;,
> > > > {, and }), so this is sufficient for the purpose of testing the output
of
> > the
> > > > core libraries.
> > > 
> > > Won't this break things?  For example, printing a string interpolation
seems
> > to
> > > call print with a single '}' to terminate the interpolation, but we don't
> want
> > > to add a new line in the middle of the string, right?
> > > 
> > >
> >
>
https://codereview.chromium.org/963593002/diff/20001/test/dart_codegen/expect...
> > > File test/dart_codegen/expect/_internal/iterable.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/963593002/diff/20001/test/dart_codegen/expect...
> > > test/dart_codegen/expect/_internal/iterable.dart:128: ";
> > > This is a bad newline break.
> > 
> > good catch!
> > 
> > I'll send a follow up CL to fix it. I could keep just new lines on ;, but I
> > thought the braces could help with readability a bit.
> > 
> > Alternatively, I can add an exception for interpolated strings. Any other
> cases
> > you can think of that we should consider?
> 
> Not sure - I'm a little nervous about this approach in that it's hard to
predict
> how someone might be using the printer (e.g. in a worst case, someone could
just
> iterate through a string of characters in a loop).  
> 
> Would it work to just add override of a few methods in UnitGeneratorCommon to
> add newlines there?  Maybe adding a new line after every statement, and a few
> other places that we're sure are valid?
> 
> Alternatively, you could hide this behind another flag - my worry is that for
> large apps we might not be able to run with dart-gen-fmt (because of time) but
> also not without it (because it hits an edge case).

since the purpose of this is just for testing for now, I've made it into a
test-only flag. I'm about to send you that change

[Jennifer Messerly, 2015-02-26 23:49:50]

https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
File lib/src/codegen/dart_codegen.dart (right):

https://codereview.chromium.org/963593002/diff/20001/lib/src/codegen/dart_cod...
lib/src/codegen/dart_codegen.dart:78: if (x == '{') {
On 2015/02/26 21:42:14, leafp wrote:
> On 2015/02/26 21:03:49, Siggi Cherem (dart-lang) wrote:
> > turns out that the ToStringVIsitor class calls print with individual symbols
> (;,
> > {, and }), so this is sufficient for the purpose of testing the output of
the
> > core libraries.
> 
> Won't this break things?  For example, printing a string interpolation seems
to
> call print with a single '}' to terminate the interpolation, but we don't want
> to add a new line in the middle of the string, right?

yeah, +1 on changing the visitor ... IMO it shouldn't be very hard, I found
analyzer's printer code to be very straightforward. I mean worst case, you could
copy+paste it and add newlines at strategic locations. But maybe I've been
working with js_ast's Printer too much lately. :)