Unify fasta->analyzer token translation logic.

Unify fasta->analyzer token translation logic.

Previously, the scanner tests and the Fasta implementation had
duplicate logic for translating Fasta tokens to analyzer tokens.  This
CL unifies the translation logic, and fills in some functionality that
was missing from the Fasta translation logic.

R=ahe@google.com

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

[Paul Berry, 2017-02-10 13:51:19]

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

[ahe, 2017-02-10 14:12:40]

lgtm

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

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:59: if
(_pseudoKeywords.contains(syntax)) {
Would token.isIdentifier or token.isPseudo work here?

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:91: "assert":
analyzer.Keyword.ASSERT,
I think you're not worried about performance right now, so feel free to ignore
this:

AFAIK, the built-in map is pretty slow, so one could try a string switch
instead. However, as far as I know, switches are implemented as a sequence of if
tests, so, at least in theory, this should be fastest:

analyzer.Keyword keywordFromString(String name) {
  if (identical("assert", name)) return analyzer.Keyword.BREAK;
  // And so on.
}

The scanner takes care to use string literals for all the keyword string, and
this is why it is safe to use identical.

[Paul Berry, 2017-02-10 17:01:02]

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

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:59: if
(_pseudoKeywords.contains(syntax)) {
On 2017/02/10 14:12:40, ahe wrote:
> Would token.isIdentifier or token.isPseudo work here?

Thanks.  token.isIdentifier is too broad (it includes things the spec calls
"built-in identifiers" such as `abstract`), but token.keyword.isPseudo is a good
suggestion.

I'm going to defer this to a follow-up CL because there are two words that the
spec, analyzer, and Fasta don't agree on how to classify (`deferred` and
`covariant`), and I want to do some more investigation and add some more tests.

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:91: "assert":
analyzer.Keyword.ASSERT,
On 2017/02/10 14:12:40, ahe wrote:
> I think you're not worried about performance right now, so feel free to ignore
> this:
> 
> AFAIK, the built-in map is pretty slow, so one could try a string switch
> instead. However, as far as I know, switches are implemented as a sequence of
if
> tests, so, at least in theory, this should be fastest:
> 
> analyzer.Keyword keywordFromString(String name) {
>   if (identical("assert", name)) return analyzer.Keyword.BREAK;
>   // And so on.
> }
> 
> The scanner takes care to use string literals for all the keyword string, and
> this is why it is safe to use identical.

You're correct that I'm not worried about performance right now, especially for
this code (I think it remains to be seen whether this translation layer is going
to be needed in the long term, or whether we will unify Fasta and analyzer's
token representations).  I've added a TODO so that if this shows up in the
profiler, we won't forget your ideas for speeding it up.

[Paul Berry, 2017-02-10 17:02:01]

Description was changed from

==========
Unify fasta->analyzer token translation logic.

Previously, the scanner tests and the Fasta implementation had
duplicate logic for translating Fasta tokens to analyzer tokens.  This
CL unifies the translation logic, and fills in some functionality that
was missing from the Fasta translation logic.
==========

to

==========
Unify fasta->analyzer token translation logic.

Previously, the scanner tests and the Fasta implementation had
duplicate logic for translating Fasta tokens to analyzer tokens.  This
CL unifies the translation logic, and fills in some functionality that
was missing from the Fasta translation logic.

R=ahe@google.com

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

[Paul Berry, 2017-02-10 17:02:02]

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

[ahe, 2017-02-10 17:10:47]

FYI

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

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:59: if
(_pseudoKeywords.contains(syntax)) {
On 2017/02/10 17:01:02, Paul Berry wrote:
> On 2017/02/10 14:12:40, ahe wrote:
> > Would token.isIdentifier or token.isPseudo work here?
> 
> Thanks.  token.isIdentifier is too broad (it includes things the spec calls
> "built-in identifiers" such as `abstract`), but token.keyword.isPseudo is a
good
> suggestion.
> 
> I'm going to defer this to a follow-up CL because there are two words that the
> spec, analyzer, and Fasta don't agree on how to classify (`deferred` and
> `covariant`), and I want to do some more investigation and add some more
tests.

I thought that might be the case.

One thing you can consider when you look at those words is to add additional
bools to the Keyword class.

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:91: "assert":
analyzer.Keyword.ASSERT,
On 2017/02/10 17:01:02, Paul Berry wrote:
> On 2017/02/10 14:12:40, ahe wrote:
> > I think you're not worried about performance right now, so feel free to
ignore
> > this:
> > 
> > AFAIK, the built-in map is pretty slow, so one could try a string switch
> > instead. However, as far as I know, switches are implemented as a sequence
of
> if
> > tests, so, at least in theory, this should be fastest:
> > 
> > analyzer.Keyword keywordFromString(String name) {
> >   if (identical("assert", name)) return analyzer.Keyword.BREAK;
> >   // And so on.
> > }
> > 
> > The scanner takes care to use string literals for all the keyword string,
and
> > this is why it is safe to use identical.
> 
> You're correct that I'm not worried about performance right now, especially
for
> this code (I think it remains to be seen whether this translation layer is
going
> to be needed in the long term, or whether we will unify Fasta and analyzer's
> token representations).  I've added a TODO so that if this shows up in the
> profiler, we won't forget your ideas for speeding it up.

FWIW, my biggest worry about unifying them is the previous token pointer. But it
should be easy to investigate the memory overhead in fasta if we add an
additional field to tokens, and it might be that bad as fasta doesn't keep them
in memory as dart2js does.

[Paul Berry, 2017-02-10 17:20:24]

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

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:59: if
(_pseudoKeywords.contains(syntax)) {
On 2017/02/10 17:10:46, ahe wrote:
> On 2017/02/10 17:01:02, Paul Berry wrote:
> > On 2017/02/10 14:12:40, ahe wrote:
> > > Would token.isIdentifier or token.isPseudo work here?
> > 
> > Thanks.  token.isIdentifier is too broad (it includes things the spec calls
> > "built-in identifiers" such as `abstract`), but token.keyword.isPseudo is a
> good
> > suggestion.
> > 
> > I'm going to defer this to a follow-up CL because there are two words that
the
> > spec, analyzer, and Fasta don't agree on how to classify (`deferred` and
> > `covariant`), and I want to do some more investigation and add some more
> tests.
> 
> I thought that might be the case.
> 
> One thing you can consider when you look at those words is to add additional
> bools to the Keyword class.

Thanks.  I was considering that, and I'm glad to hear that your thoughts run in
a similar direction.

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:91: "assert":
analyzer.Keyword.ASSERT,
On 2017/02/10 17:10:46, ahe wrote:
> On 2017/02/10 17:01:02, Paul Berry wrote:
> > On 2017/02/10 14:12:40, ahe wrote:
> > > I think you're not worried about performance right now, so feel free to
> ignore
> > > this:
> > > 
> > > AFAIK, the built-in map is pretty slow, so one could try a string switch
> > > instead. However, as far as I know, switches are implemented as a sequence
> of
> > if
> > > tests, so, at least in theory, this should be fastest:
> > > 
> > > analyzer.Keyword keywordFromString(String name) {
> > >   if (identical("assert", name)) return analyzer.Keyword.BREAK;
> > >   // And so on.
> > > }
> > > 
> > > The scanner takes care to use string literals for all the keyword string,
> and
> > > this is why it is safe to use identical.
> > 
> > You're correct that I'm not worried about performance right now, especially
> for
> > this code (I think it remains to be seen whether this translation layer is
> going
> > to be needed in the long term, or whether we will unify Fasta and analyzer's
> > token representations).  I've added a TODO so that if this shows up in the
> > profiler, we won't forget your ideas for speeding it up.
> 
> FWIW, my biggest worry about unifying them is the previous token pointer. But
it
> should be easy to investigate the memory overhead in fasta if we add an
> additional field to tokens, and it might be that bad as fasta doesn't keep
them
> in memory as dart2js does.

Agreed--it's a complex tradeoff.  I don't intend to make any movements in that
direction without gathering data and opinions first :)

[ahe, 2017-02-10 17:22:46]

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

https://codereview.chromium.org/2682423003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/token_utils.dart:91: "assert":
analyzer.Keyword.ASSERT,
On 2017/02/10 17:20:24, Paul Berry wrote:
> On 2017/02/10 17:10:46, ahe wrote:
> > On 2017/02/10 17:01:02, Paul Berry wrote:
> > > On 2017/02/10 14:12:40, ahe wrote:
> > > > I think you're not worried about performance right now, so feel free to
> > ignore
> > > > this:
> > > > 
> > > > AFAIK, the built-in map is pretty slow, so one could try a string switch
> > > > instead. However, as far as I know, switches are implemented as a
sequence
> > of
> > > if
> > > > tests, so, at least in theory, this should be fastest:
> > > > 
> > > > analyzer.Keyword keywordFromString(String name) {
> > > >   if (identical("assert", name)) return analyzer.Keyword.BREAK;
> > > >   // And so on.
> > > > }
> > > > 
> > > > The scanner takes care to use string literals for all the keyword
string,
> > and
> > > > this is why it is safe to use identical.
> > > 
> > > You're correct that I'm not worried about performance right now,
especially
> > for
> > > this code (I think it remains to be seen whether this translation layer is
> > going
> > > to be needed in the long term, or whether we will unify Fasta and
analyzer's
> > > token representations).  I've added a TODO so that if this shows up in the
> > > profiler, we won't forget your ideas for speeding it up.
> > 
> > FWIW, my biggest worry about unifying them is the previous token pointer.
But
> it
> > should be easy to investigate the memory overhead in fasta if we add an
> > additional field to tokens, and it might be that bad as fasta doesn't keep
> them
> > in memory as dart2js does.
> 
> Agreed--it's a complex tradeoff.  I don't intend to make any movements in that
> direction without gathering data and opinions first :)

Data and opinions are good :-)

Just to be clear, I left out a negation above. I think the memory overhead might
*not* be that bad.