Implement shell-style comments and record shebangs as comments.

Record shebangs as comments.

R=paulberry@google.com

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

[ahe, 2017-02-21 10:30:46]

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

[ahe, 2017-02-21 10:30:46]

Paul, I think you have a todo about script tags that this should address. It
might make sense to generate a different token for script tags, currently it
will be a comment token.

The reason why I'm enabling shell-style comments is that I'd like to experiment
reading .packages files using the scanner as I'm currently troubled by the
performance of package:package_config.

[Paul Berry, 2017-02-21 13:47:32]

On 2017/02/21 10:30:46, ahe wrote:
> Paul, I think you have a todo about script tags that this should address. It
> might make sense to generate a different token for script tags, currently it
> will be a comment token.
> 
> The reason why I'm enabling shell-style comments is that I'd like to
experiment
> reading .packages files using the scanner as I'm currently troubled by the
> performance of package:package_config.

Actually I don't think this CL makes a difference for my TODO about script tags.
 AFAICT, this CL just affects the behavior of the Fasta scanner when "#" appears
*after* the beginning of the file.  Analyzer only needs script tags to be
recognized if they're at the top of the file, which I see from the diffs has
always been the case.

(Sorry if my communication about this, including my TODO comments, has been
unclear in the past--I didn't realize the scanner treated script tags as
comments).

I see your point that it might make sense to generate a different token for
script tags, since analyzer expects the parser to convert a script tag into an
AST construct.  However, it might be less work to just keep the current behavior
(script tag is a comment and is ignored by the parser) and add a special
behavior when creating analyzer ASTs that looks for the script tag "comment" and
converts it to an AST node.  I'm happy to go with your recommendation.

[Paul Berry, 2017-02-21 13:50:22]

On 2017/02/21 13:47:32, Paul Berry wrote:
> On 2017/02/21 10:30:46, ahe wrote:
> > Paul, I think you have a todo about script tags that this should address. It
> > might make sense to generate a different token for script tags, currently it
> > will be a comment token.
> > 
> > The reason why I'm enabling shell-style comments is that I'd like to
> experiment
> > reading .packages files using the scanner as I'm currently troubled by the
> > performance of package:package_config.
> 
> Actually I don't think this CL makes a difference for my TODO about script
tags.
>  AFAICT, this CL just affects the behavior of the Fasta scanner when "#"
appears
> *after* the beginning of the file.  Analyzer only needs script tags to be
> recognized if they're at the top of the file, which I see from the diffs has
> always been the case.
> 
> (Sorry if my communication about this, including my TODO comments, has been
> unclear in the past--I didn't realize the scanner treated script tags as
> comments).

Oops, sorry.  Disregard the above part of my previous response.  I misread the
code.  Let me try again:

Yes, this provides the scanner logic that we would need to recognize script
tags.  Thank you :)

I think the paragraph below still makes sense:

> I see your point that it might make sense to generate a different token for
> script tags, since analyzer expects the parser to convert a script tag into an
> AST construct.  However, it might be less work to just keep the current
behavior
> (script tag is a comment and is ignored by the parser) and add a special
> behavior when creating analyzer ASTs that looks for the script tag "comment"
and
> converts it to an AST node.  I'm happy to go with your recommendation.

[ahe, 2017-02-21 14:33:38]

On 2017/02/21 13:47:32, Paul Berry wrote:
> I see your point that it might make sense to generate a different token for
> script tags, since analyzer expects the parser to convert a script tag into an
> AST construct.  However, it might be less work to just keep the current
behavior
> (script tag is a comment and is ignored by the parser) and add a special
> behavior when creating analyzer ASTs that looks for the script tag "comment"
and
> converts it to an AST node.  I'm happy to go with your recommendation.

What I was thinking was to create a new kind of token for this, but make it
conditional on includeComments. So I should update the documentation of that
field.

[ahe, 2017-02-21 16:57:10]

https://codereview.chromium.org/2711453002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/abstract_scanner.dart (right):

https://codereview.chromium.org/2711453002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/abstract_scanner.dart:38: final bool
enableShellStyleComments;
I should probably remove this again. After thinking a bit more about it, I don't
think it makes sense to attempt parsing .packages files or YAML files with this
scanner.

[Paul Berry, 2017-02-21 16:59:33]

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

[Paul Berry, 2017-02-21 16:59:34]

https://codereview.chromium.org/2711453002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/abstract_scanner.dart (right):

https://codereview.chromium.org/2711453002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/abstract_scanner.dart:38: final bool
enableShellStyleComments;
On 2017/02/21 16:57:10, ahe wrote:
> I should probably remove this again. After thinking a bit more about it, I
don't
> think it makes sense to attempt parsing .packages files or YAML files with
this
> scanner.

SGTM.  I admit I was surprised you were heading down that path in the first
place :)

[ahe, 2017-02-21 17:37:34]

https://codereview.chromium.org/2711453002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/abstract_scanner.dart (right):

https://codereview.chromium.org/2711453002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/abstract_scanner.dart:38: final bool
enableShellStyleComments;
On 2017/02/21 16:59:34, Paul Berry wrote:
> On 2017/02/21 16:57:10, ahe wrote:
> > I should probably remove this again. After thinking a bit more about it, I
> don't
> > think it makes sense to attempt parsing .packages files or YAML files with
> this
> > scanner.
> 
> SGTM.  I admit I was surprised you were heading down that path in the first
> place :)

Yeah, it feels like a stupid idea now :-)

Sometimes I need things to be very concrete before I really understand it.

[ahe, 2017-02-27 08:45:51]

Description was changed from

==========
Implement shell-style comments and record shebangs as comments.
==========

to

==========
Record shebangs as comments.
==========

[ahe, 2017-02-27 08:46:15]

I've removed the shell-style part, and updated the description to match.

PTAL

[Paul Berry, 2017-02-27 13:55:27]

lgtm

[ahe, 2017-02-27 14:32:45]

Description was changed from

==========
Record shebangs as comments.
==========

to

==========
Record shebangs as comments.

R=paulberry@google.com

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

[ahe, 2017-02-27 14:32:47]

Committed patchset #3 (id:40001) manually as
c4045d4ce8f7c72545975e188076bee76be769da (presubmit successful).