Modify scanner and parser to be standalone packages.

Modify scanner and parser to be standalone packages.

Also, minor changes to support new compiler.

R=johnniwinther@google.com, sigmund@google.com

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

[ahe, 2017-01-12 10:30:52]

Patchset #1 (id:1) has been deleted

[ahe, 2017-01-12 10:31:26]

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

[Johnni Winther, 2017-01-12 13:14:38]

lgtm

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/src...
File pkg/dart_parser/lib/src/main.dart (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/src...
pkg/dart_parser/lib/src/main.dart:55: new TopLevelParser(new
DebugListener()).parseUnit(
Should the DebugListener be used unconditionally?

[ahe, 2017-01-12 13:57:54]

Thank you, Johnni.

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/src...
File pkg/dart_parser/lib/src/main.dart (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/src...
pkg/dart_parser/lib/src/main.dart:55: new TopLevelParser(new
DebugListener()).parseUnit(
On 2017/01/12 13:14:37, Johnni Winther wrote:
> Should the DebugListener be used unconditionally?

I think so. This program isn't worth much without that listener. With the debug
listener it prints the events, without it it doesn't do much (except if there is
an error).

[Siggi Cherem (dart-lang), 2017-01-12 21:56:03]

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

[Siggi Cherem (dart-lang), 2017-01-12 21:56:04]

lgtm - none of my comments require action on this CL

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/par...
File pkg/dart_parser/lib/parser.dart (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/par...
pkg/dart_parser/lib/parser.dart:7: import 'package:scanner/src/token.dart' show
It doesn't have to be done now on this CL since you likely have a pile of CLs
stacked on top of this that will have conflicts, but at some point before we
integrate into master we should run dartfmt on all the files.

It took us time to switch in dart2js, but now we really enjoy delegating
formatting to it.

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/par...
pkg/dart_parser/lib/parser.dart:22: List<ParserError> parse(Token tokens, {bool
enableGenericMethodSyntax: false}) {
also can be done on a later CL, but the enableGenericMethodSyntax flag is now
gone and defaults to true.

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/src...
File pkg/dart_parser/lib/src/class_member_parser.dart (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/src...
pkg/dart_parser/lib/src/class_member_parser.dart:49: Token skipExpression(Token
token) {
side comment, also can be ignored for this CL: it would be nice to move skip* to
the base class, or add another intermediate class that contains the skip methods
without overriding the default behavior.

This came up when I was playing with the summary builder: I wanted to extend the
base Parser to sometimes to parseExpressions and sometimes do skipExpressions,
and the only way I could do so was to copy some of this code. I no longer need
to do that, so this is not something needed at this very moment.

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/test/pa...
File pkg/dart_parser/test/parser_suite.dart (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/test/pa...
pkg/dart_parser/test/parser_suite.dart:5: import 'package:testing/testing.dart';
I'm curious to take a look at this package in more detail. Is it already in
fe_integration or CLs where I could take a look?

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/testing...
File pkg/dart_parser/testing.json (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/testing...
pkg/dart_parser/testing.json:10: "path": "../../",
It's nice that you can use the entire repo as a test suite! It however seems
brittle that anyone submitting a Dart file could technically make the test suite
fail. Is that a concern? I guess most of the time Dart files are valid and it
should be OK.

[ahe, 2017-01-13 07:16:21]

Thank you, both!

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/par...
File pkg/dart_parser/lib/parser.dart (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/par...
pkg/dart_parser/lib/parser.dart:7: import 'package:scanner/src/token.dart' show
On 2017/01/12 21:56:04, Siggi Cherem (dart-lang) wrote:
> It doesn't have to be done now on this CL since you likely have a pile of CLs
> stacked on top of this that will have conflicts, but at some point before we
> integrate into master we should run dartfmt on all the files.
> 
> It took us time to switch in dart2js, but now we really enjoy delegating
> formatting to it.

The way dartfmt formats show clauses slow me down. It makes it harder to see how
show clauses have changed when reviewing code, and it makes it harder for me to
add new items.

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/par...
pkg/dart_parser/lib/parser.dart:22: List<ParserError> parse(Token tokens, {bool
enableGenericMethodSyntax: false}) {
On 2017/01/12 21:56:04, Siggi Cherem (dart-lang) wrote:
> also can be done on a later CL, but the enableGenericMethodSyntax flag is now
> gone and defaults to true.

Yes. I'll address this when I have landed my other changes and merge with
changes to the parser since I copied it.

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/src...
File pkg/dart_parser/lib/src/class_member_parser.dart (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/lib/src...
pkg/dart_parser/lib/src/class_member_parser.dart:49: Token skipExpression(Token
token) {
On 2017/01/12 21:56:04, Siggi Cherem (dart-lang) wrote:
> side comment, also can be ignored for this CL: it would be nice to move skip*
to
> the base class, or add another intermediate class that contains the skip
methods
> without overriding the default behavior.
> 
> This came up when I was playing with the summary builder: I wanted to extend
the
> base Parser to sometimes to parseExpressions and sometimes do skipExpressions,
> and the only way I could do so was to copy some of this code. I no longer need
> to do that, so this is not something needed at this very moment.

Yes. That's a great idea. I've created a reminder in my inbox for that.

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/test/pa...
File pkg/dart_parser/test/parser_suite.dart (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/test/pa...
pkg/dart_parser/test/parser_suite.dart:5: import 'package:testing/testing.dart';
On 2017/01/12 21:56:04, Siggi Cherem (dart-lang) wrote:
> I'm curious to take a look at this package in more detail. Is it already in
> fe_integration or CLs where I could take a look?

I have outstanding reviews for landing it, but you can see most of it here:
https://github.com/peter-ahe-google/testing. In addition, take a look at CL
2627723006 and follow its dependencies (or just look at my open code reviews, I
just cleaned up the list).

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/testing...
File pkg/dart_parser/testing.json (right):

https://codereview.chromium.org/2631503002/diff/20001/pkg/dart_parser/testing...
pkg/dart_parser/testing.json:10: "path": "../../",
On 2017/01/12 21:56:04, Siggi Cherem (dart-lang) wrote:
> It's nice that you can use the entire repo as a test suite! It however seems
> brittle that anyone submitting a Dart file could technically make the test
suite
> fail. Is that a concern? I guess most of the time Dart files are valid and it
> should be OK.

That is a big concern, and something I'll have to carefully consider before
setting up automated tests based on this.

But the great thing is that this config file could be something we just use
occasionally, or run on a separate build bot.

[ahe, 2017-01-13 07:17:12]

Description was changed from

==========
Modify scanner and parser to be standalone packages.

Also, minor changes to support new compiler.
==========

to

==========
Modify scanner and parser to be standalone packages.

Also, minor changes to support new compiler.

R=johnniwinther@google.com, sigmund@google.com

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

[ahe, 2017-01-13 07:17:14]

Committed patchset #1 (id:20001) manually as
6edda4bb5e906a9732d9e0422470f52a5b774153 (presubmit successful).