Add AstBuilder support for class declarations and named mixin applications.

Add AstBuilder support for class declarations and named mixin applications.

R=scheglov@google.com

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

[Paul Berry, 2017-02-17 22:40:50]

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

[scheglov, 2017-02-17 22:48:11]

LGTM

[Paul Berry, 2017-02-17 23:07:00]

Description was changed from

==========
Add AstBuilder support for class declarations and named mixin applications.
==========

to

==========
Add AstBuilder support for class declarations and named mixin applications.

R=scheglov@google.com

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

[Paul Berry, 2017-02-17 23:07:02]

Committed patchset #1 (id:1) manually as
a047dd15c9af336f956777a71403a301935366fe (presubmit successful).

[ahe, 2017-02-20 13:04:39]

lgtm

No action items.

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:777: // keyword.
No problem. If you file bugs with lack of token information in parser events,
I'll fix them.

When I change the signature of an event, I have to modify dart2js as well, so it
takes a little longer to run tests. This is why I prefer having a bug and take
them in batches.

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:778: if
(identical(beginToken.value, 'abstract')) {
I suggest using optional like this:

if (optional("abstract", token)) {
...
}

This method is exported by ../parser/parser.dart

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:835: // keyword.
I agree with all these comments about the parser giving access to tokens.

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:865: ///
[ClassDeclaration] object.
An alternative implementation is to simply push more values on the stack.

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/listener.dart:262: /// the number of
implemented types is passed as a parameter.
I agree. I would favor passing in a count here.

[Paul Berry, 2017-02-20 14:25:11]

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:777: // keyword.
On 2017/02/20 13:04:39, ahe wrote:
> No problem. If you file bugs with lack of token information in parser events,
> I'll fix them.
> 
> When I change the signature of an event, I have to modify dart2js as well, so
it
> takes a little longer to run tests. This is why I prefer having a bug and take
> them in batches.

Ok.  I'm still planning to go through all my CLs collecting issues to file; when
I do that I'll make sure to capture issues like this.  I'll try to get to that
tomorrow at the latest.

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:778: if
(identical(beginToken.value, 'abstract')) {
On 2017/02/20 13:04:39, ahe wrote:
> I suggest using optional like this:
> 
> if (optional("abstract", token)) {
> ...
> }
> 
> This method is exported by ../parser/parser.dart

Acknowledged.

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:865: ///
[ClassDeclaration] object.
On 2017/02/20 13:04:39, ahe wrote:
> An alternative implementation is to simply push more values on the stack.

Acknowledged.  I think I prefer using the temporary object, though, because it
lets me keep the invariant that every "handle..." method pops one element off
the stack per substructure.  If we start letting a substructure be represented
by multiple stack entries, then I have to think harder when I write a
"handle..." method to figure out which stack entry is which.

But if this data structure proves to be too much of a performance burden, I'll
definitely consider your suggestion.

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/parser/listener.dart (right):

https://codereview.chromium.org/2703863003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/listener.dart:262: /// the number of
implemented types is passed as a parameter.
On 2017/02/20 13:04:39, ahe wrote:
> I agree. I would favor passing in a count here.

SGTM.  I don't think I have a preference.