Refactor parseType in preparation for bigger changes.

Refactor parseType in preparation for bigger changes.

R=danrubel@google.com

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

[ahe, 2017-06-19 14:40:51]

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

[ahe, 2017-06-19 14:40:52]

Changes I'm preparing for:

* void can be use as any other type.

* major rewrite of how function types are parsed to solve issue 29798.

[danrubel, 2017-06-19 15:31:06]

LGTM with comments for you to address as you see fit.

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

https://codereview.chromium.org/2951463002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1143: if (isOptional) {
for better performance, should this block be placed into a parseTypeSomething
which then calls parseType?

https://codereview.chromium.org/2951463002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1167: } while (false);
Why is all this in a do while(false)? It does not feel or look like a loop so
this structure is confusing. I recommend refactoring this as if/else-if/else
blocks.

[ahe, 2017-06-19 16:46:49]

Thank you, Dan!

Your comments are spot on, but I have much bigger plans for this, so I expect to
completely rewrite parseType.

I also want to get rid of the peekAfter methods.

Here's a bit of details about what I'm thinking:

I want to be able to call parseType unconditionally. This could potentially mean
that I have to pass a continuation to it for use in parsing expression
statements.

I want to handle generic type parameters better. I want to be able to call
beginFunctionType before handleType for its return type. Consider this example:

  T Function<R>(/*1*/) Function<S>(/*2*/) Function<T>(/*3*/)

Currently, fasta can't resolve T correctly, and I want to fix this by changing
the parser so it generates these events:

endTypeVariables // <R>
endTypeVariables // <S>
endTypeVariables // <T>
beginFunctionType /*3*/
beginFunctionType /*2*/
beginFunctionType /*1*/
endType // T
endArguments /*1*/
endFunctionType /*1*/
endType // T Function<R>(/*1*/)
endArguments /*2*/
endFunctionType /*2*/
endType // T Function<R>(/*1*/) Function<2>(/*2*/)
endArguments /*3*/
endFunctionType /*3*/
endType // T Function<R>(/*1*/) Function<S>(/*2*/) Function<T>(/*3*/)

This requires a loop and potentially recursion, but I think I can get rid of all
the peeking.

In addition, Martin suggested that parsing stuff like "var x;" could be
simplified and potentially make the parser much faster.

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

https://codereview.chromium.org/2951463002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1143: if (isOptional) {
On 2017/06/19 15:31:06, danrubel wrote:
> for better performance, should this block be placed into a parseTypeSomething
> which then calls parseType?

Great idea, but I have much bigger plans for this :-)

https://codereview.chromium.org/2951463002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/parser/parser.dart:1167: } while (false);
On 2017/06/19 15:31:06, danrubel wrote:
> Why is all this in a do while(false)? It does not feel or look like a loop so
> this structure is confusing. I recommend refactoring this as if/else-if/else
> blocks.

It was for the breaks. I agree it isn't nice, but again, much bigger plans for
this :-)

[danrubel, 2017-06-19 17:00:49]

On 2017/06/19 16:46:49, ahe wrote:
> Thank you, Dan!
> 
> Your comments are spot on, but I have much bigger plans for this, so I expect
to
> completely rewrite parseType.
> 
> I also want to get rid of the peekAfter methods.
> 
> Here's a bit of details about what I'm thinking:
> 
> I want to be able to call parseType unconditionally. This could potentially
mean
> that I have to pass a continuation to it for use in parsing expression
> statements.
> 
> I want to handle generic type parameters better. I want to be able to call
> beginFunctionType before handleType for its return type. Consider this
example:
> 
>   T Function<R>(/*1*/) Function<S>(/*2*/) Function<T>(/*3*/)
> 
> Currently, fasta can't resolve T correctly, and I want to fix this by changing
> the parser so it generates these events:
> 
> endTypeVariables // <R>
> endTypeVariables // <S>
> endTypeVariables // <T>
> beginFunctionType /*3*/
> beginFunctionType /*2*/
> beginFunctionType /*1*/
> endType // T
> endArguments /*1*/
> endFunctionType /*1*/
> endType // T Function<R>(/*1*/)
> endArguments /*2*/
> endFunctionType /*2*/
> endType // T Function<R>(/*1*/) Function<2>(/*2*/)
> endArguments /*3*/
> endFunctionType /*3*/
> endType // T Function<R>(/*1*/) Function<S>(/*2*/) Function<T>(/*3*/)
> 
> This requires a loop and potentially recursion, but I think I can get rid of
all
> the peeking.
> 
> In addition, Martin suggested that parsing stuff like "var x;" could be
> simplified and potentially make the parser much faster.
> 
>
https://codereview.chromium.org/2951463002/diff/1/pkg/front_end/lib/src/fasta...
> File pkg/front_end/lib/src/fasta/parser/parser.dart (right):
> 
>
https://codereview.chromium.org/2951463002/diff/1/pkg/front_end/lib/src/fasta...
> pkg/front_end/lib/src/fasta/parser/parser.dart:1143: if (isOptional) {
> On 2017/06/19 15:31:06, danrubel wrote:
> > for better performance, should this block be placed into a
parseTypeSomething
> > which then calls parseType?
> 
> Great idea, but I have much bigger plans for this :-)
> 
>
https://codereview.chromium.org/2951463002/diff/1/pkg/front_end/lib/src/fasta...
> pkg/front_end/lib/src/fasta/parser/parser.dart:1167: } while (false);
> On 2017/06/19 15:31:06, danrubel wrote:
> > Why is all this in a do while(false)? It does not feel or look like a loop
so
> > this structure is confusing. I recommend refactoring this as if/else-if/else
> > blocks.
> 
> It was for the breaks. I agree it isn't nice, but again, much bigger plans for
> this :-)

Sounds good. LGTM !

[ahe, 2017-06-22 10:47:58]

Description was changed from

==========
Refactor parseType in preparation for bigger changes.
==========

to

==========
Refactor parseType in preparation for bigger changes.

R=danrubel@google.com

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

[ahe, 2017-06-22 10:47:59]

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