Remove peeking from parseTypedef and parseClassOrNamedMixinApplication.

Remove peeking from parseTypedef and parseClassOrNamedMixinApplication.

R=paulberry@google.com

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

[ahe, 2017-06-23 14:33:21]

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

[ahe, 2017-06-23 14:33:22]

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/outline_builder.dart (left):

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:413:
library.beginNestedDeclaration(name.lexeme, hasMembers: false);
hasMember is mostly an optimization, so it's safe to use true for named mixin
applications.

[ahe, 2017-06-24 10:10:31]

Not asking you to review this over the weekend.

There was just something on my mind that I wanted to write down now.

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

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1138: if (continuation != null) {
A little bit of context here:

My plan is to move all the peeking to this method (parseType). Once that is
complete, it should be easier to see how to rewrite it to achieve my longer term
goal: solve the type variable scope issue for generalized function types and
improve performance of parser by reducing look ahead.

So this is going to look more and more horrible, as I move stuff here. But it's
horrible stuff that's currently just sprinkled throughout the parser. So the
total measure of horribleness such remain the same :-)

[Paul Berry, 2017-06-24 14:16:26]

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

[Paul Berry, 2017-06-24 14:16:26]

lgtm

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

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:183: enum TypeContinuation {
It would be nice to have a comment explaining what this is for.  Here is the
best I've thought of (feel free to use this text or adapt it as you see fit). 
Nit: also consider renaming this to "TypeContext".

/// Indicates to [Parser.parseType] the context in which a type is to
/// be parsed; this determines the circumstances in which the attempt
/// to parse a type should be aborted (because the construct is
/// actually not a type).
enum TypeContinuation {
  /// The construct to be parsed is at the beginning of a typedef; if
  /// an identifier followed by an equals sign is seen, then type
  /// parsing should be aborted.
  Typedef,
}

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1136: Token parseType(Token
token,
It would be nice to have a comment here explaining the function of
`continuation` as well as when `null` might be returned.  E.g.:

/// Parses a type (if it is appropriate to do so).
///
/// [continuation] indicates the context in which the type is being
/// parsed.  If the parser determines that the tokens do not actually
/// constitute a type in this context, then `null` is returned and the
/// caller should attempt to parse the tokens in some other way.

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1139: switch (continuation) {
Nit: It seems odd to have a null check followed by a switch statement on an
enum.  Personally, I think it would be more natural to add another possible
value to the enum which indicates the default behavior.  That way `continuation`
will never be null.

[ahe, 2017-06-26 10:06:10]

Patchset #2 (id:20001) has been deleted

[ahe, 2017-06-26 10:11:14]

Patchset #2 (id:40001) has been deleted

[ahe, 2017-06-26 10:14:01]

Thank you, Paul. I took your ideas and extended them a little. I'm going to land
this now, but please take a second look at my changes.

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

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:183: enum TypeContinuation {
On 2017/06/24 14:16:26, Paul Berry wrote:
> It would be nice to have a comment explaining what this is for.  Here is the
> best I've thought of (feel free to use this text or adapt it as you see fit). 
> Nit: also consider renaming this to "TypeContext".

I want to reserve that word for you :-)

> /// Indicates to [Parser.parseType] the context in which a type is to
> /// be parsed; this determines the circumstances in which the attempt
> /// to parse a type should be aborted (because the construct is
> /// actually not a type).
> enum TypeContinuation {
>   /// The construct to be parsed is at the beginning of a typedef; if
>   /// an identifier followed by an equals sign is seen, then type
>   /// parsing should be aborted.
>   Typedef,
> }

Done.

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1136: Token parseType(Token
token,
On 2017/06/24 14:16:26, Paul Berry wrote:
> It would be nice to have a comment here explaining the function of
> `continuation` as well as when `null` might be returned.  E.g.:
> 
> /// Parses a type (if it is appropriate to do so).
> ///
> /// [continuation] indicates the context in which the type is being
> /// parsed.  If the parser determines that the tokens do not actually
> /// constitute a type in this context, then `null` is returned and the
> /// caller should attempt to parse the tokens in some other way.

Done.

https://codereview.chromium.org/2948383002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1139: switch (continuation) {
On 2017/06/24 14:16:26, Paul Berry wrote:
> Nit: It seems odd to have a null check followed by a switch statement on an
> enum.  Personally, I think it would be more natural to add another possible
> value to the enum which indicates the default behavior.  That way
`continuation`
> will never be null.

Done.

[ahe, 2017-06-26 11:15:53]

Description was changed from

==========
Remove peeking from parseTypedef and parseClassOrNamedMixinApplication.
==========

to

==========
Remove peeking from parseTypedef and parseClassOrNamedMixinApplication.

R=paulberry@google.com

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

[ahe, 2017-06-26 11:15:55]

Committed patchset #2 (id:60001) manually as
a1ad4e4feb858dfcc2078067e644098b7d839c75 (presubmit successful).

[Paul Berry, 2017-06-26 13:27:35]

lgtm