Scanner can include comments in the token stream.

Scanner can include comments in the token stream.
includeComments option argument added to the ByteArrayScanner. default is false.
Test of charOffset, charLength invariants on scanner output added.
Calls to beginToken() moved to fix invalid Token.charOffset values for string interpolations.


BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=9059

[Anton Muhin, 2012-06-06 16:18:35]

DBC

If I correctly understand what goes on here, it may break
http://code.google.com/p/dart/source/detail?r=8314

something like 

  operator /**/ []

would be incorrectly parsed

[Johnni Winther, 2012-06-07 08:22:16]

http://codereview.chromium.org/10539021/diff/1/lib/compiler/implementation/sc...
File lib/compiler/implementation/scanner/scanner.dart (right):

http://codereview.chromium.org/10539021/diff/1/lib/compiler/implementation/sc...
lib/compiler/implementation/scanner/scanner.dart:715:
appendBeginGroup(STRING_INTERPOLATION_INFO, "\$");
There seems to be a problem with using "\$" and not "\${" as the value for the
generated BeginGroupToken since Token.stringValue depends on it.

http://codereview.chromium.org/10539021/diff/1/lib/compiler/implementation/sc...
lib/compiler/implementation/scanner/scanner.dart:719:
appendEndGroup(CLOSE_CURLY_BRACKET_INFO, "", OPEN_CURLY_BRACKET_TOKEN);
There seems to be a problem with using "" and not "}" as the value for the
generated StringToken since Token.stringValue depends on it.

[Johnni Winther, 2012-06-07 08:29:56]

Inclusion of comments is optional and off by default so

  operator /**/ []

generates the tokens

  operator []

[ahe, 2012-06-15 13:34:28]

I think we need to find a different approach to Anton's fix.

Anton, do you have test case?

Cheers,
Peter

[Johnni Winther, 2012-06-21 13:33:42]

A new patch has been uploaded.

[ahe, 2012-06-22 08:42:49]

Definitely a step in the right direction, but I have a whole bunch of style nits
and a few more substantial concerns (that should be easy to fix).

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
File lib/compiler/implementation/scanner/byte_array_scanner.dart (right):

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/byte_array_scanner.dart:12:
ByteArrayScanner(List<int> this.bytes, [bool includeComments = false]) :
super(includeComments);
Line is longer than 80 characters.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
File lib/compiler/implementation/scanner/parser.dart (right):

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/parser.dart:1296: } else if (optional('\$',
token)) {
I don't like testing for '$' as it can also be an identifier. I would be more
comfortable with a new token kind.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/parser.dart:1299: token = expect('', token);
I think we can avoid creating a group in this case. So you should be able to get
rid of this line.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
File lib/compiler/implementation/scanner/scanner.dart (right):

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:15: /**
Please add a newline before all the documentation comments.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:16: * Returns the current char.
No it doesn't :-)

I think the word you're looking for is "character", but what it really returns
is the next byte.

And that's not entirely true either. If this is StringScanner, it returns the
next character. If this is ByteArrayScanner, it returns the next byte.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:34: * Appends a token whose
kind is determined by [info] and content is defined by
Long line.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:47: * the scanner. I.e. if the
current byte offset is 10, [:asciiString(0,-1):]
A tech-writer once recommended that I avoided the use of "i.e." and "e.g.".
Apparently, those terms are confusing to many people. Instead, use "that is" and
"for example", respectively.

Also, I don't think you start a sentence with "i.e" or "that is". But I could be
wrong. However, I believe that all of these phrases should be followed by a
comma or colon.

In this case, I think you're giving an example, so it should be "for example".

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:574: if ($LF === next || $CR
=== next || $EOF === next) {
Should we include DOS line endings?

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:722: beginToken(); // $ starts
here
Not a proper sentence. A proper sentence starts with an uppercase letter and is
terminated by period.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:733: beginToken(); //
expression starts here
Not proper sentence.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:740: beginToken(); // string
interpolation suffix starts here
Ditto.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:746: beginToken(); //
identifier starts here
Ditto.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:748: beginToken(); // string
interpolation suffix starts here
Ditto.

https://chromiumcodereview.appspot.com/10539021/diff/10002/lib/compiler/imple...
lib/compiler/implementation/scanner/scanner.dart:749:
appendEndGroup(CLOSE_CURLY_BRACKET_INFO, "", OPEN_CURLY_BRACKET_TOKEN);
This is not a CLOSE_CURLE_BRACKET.

[Johnni Winther, 2012-06-22 09:55:52]

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/scanner.dart (right):

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:574: if ($LF === next || $CR
=== next || $EOF === next) {
On 2012/06/22 08:42:49, ahe wrote:
> Should we include DOS line endings?

The terminating character is not included in the single line comment but instead
tokenized as whitespace (by bigSwitch), which therefore handles DOS endings as
well.

[Johnni Winther, 2012-06-22 10:23:10]

String interpolation of identifiers now uses a new token as marker.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1296: } else if (optional('\$',
token)) {
On 2012/06/22 08:42:49, ahe wrote:
> I don't like testing for '$' as it can also be an identifier. I would be more
> comfortable with a new token kind.
Changed to use the new STRING_INTERPOLATION_IDENTIFIER_TOKEN instead.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1299: token = expect('', token);
On 2012/06/22 08:42:49, ahe wrote:
> I think we can avoid creating a group in this case. So you should be able to
get
> rid of this line.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/scanner.dart (right):

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:15: /**
On 2012/06/22 08:42:49, ahe wrote:
> Please add a newline before all the documentation comments.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:16: * Returns the current char.
On 2012/06/22 08:42:49, ahe wrote:
> No it doesn't :-)
> 
> I think the word you're looking for is "character", but what it really returns
> is the next byte.
> 
> And that's not entirely true either. If this is StringScanner, it returns the
> next character. If this is ByteArrayScanner, it returns the next byte.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:34: * Appends a token whose
kind is determined by [info] and content is defined by
On 2012/06/22 08:42:49, ahe wrote:
> Long line.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:47: * the scanner. I.e. if the
current byte offset is 10, [:asciiString(0,-1):]
On 2012/06/22 08:42:49, ahe wrote:
> A tech-writer once recommended that I avoided the use of "i.e." and "e.g.".
> Apparently, those terms are confusing to many people. Instead, use "that is"
and
> "for example", respectively.
> 
> Also, I don't think you start a sentence with "i.e" or "that is". But I could
be
> wrong. However, I believe that all of these phrases should be followed by a
> comma or colon.
> 
> In this case, I think you're giving an example, so it should be "for example".

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:722: beginToken(); // $ starts
here
On 2012/06/22 08:42:49, ahe wrote:
> Not a proper sentence. A proper sentence starts with an uppercase letter and
is
> terminated by period.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:733: beginToken(); //
expression starts here
On 2012/06/22 08:42:49, ahe wrote:
> Not proper sentence.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:740: beginToken(); // string
interpolation suffix starts here
On 2012/06/22 08:42:49, ahe wrote:
> Ditto.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:746: beginToken(); //
identifier starts here
On 2012/06/22 08:42:49, ahe wrote:
> Ditto.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:748: beginToken(); // string
interpolation suffix starts here
On 2012/06/22 08:42:49, ahe wrote:
> Ditto.

Done.

http://codereview.chromium.org/10539021/diff/10002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:749:
appendEndGroup(CLOSE_CURLY_BRACKET_INFO, "", OPEN_CURLY_BRACKET_TOKEN);
On 2012/06/22 08:42:49, ahe wrote:
> This is not a CLOSE_CURLE_BRACKET.

Done.

http://codereview.chromium.org/10539021/diff/16002/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/scanner.dart (right):

http://codereview.chromium.org/10539021/diff/16002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/scanner.dart:755:
appendPrecedenceToken(STRING_INTERPOLATION_IDENTIFIER_INFO);
Changed to use the new STRING_INTERPOLATION_IDENTIFIER_INFO and avoid creating a
group at this point.

[Johnni Winther, 2012-06-22 11:43:11]

Parsing of general string interpolation now also uses token.kind test instead of
optional(...).

[ahe, 2012-06-25 08:12:37]

I understand the scanner changes, and they look good.

I'm concerned about wrapping slowToString in a method that isn't called
slow-something.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/parser.dart:1289: var tokenKind =
token.kind;
In other places I have used:

var kind = token.kind;

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/parser.dart:1296: token =
parseStringPart(token);
The last two lines can be shared.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
File lib/compiler/implementation/scanner/scanner.dart (right):

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/scanner.dart:19: * characters whereas
[ByteArrayScanner] operators on byte arrays and thus
... characters (Unicode codepoints represented as int) ...

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/scanner.dart:28: * and content is determined
by [no] is appended. */
*/ on its own line.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/scanner.dart:55: * Creates an ascii
SourceString whose content begins at the source byte
ascii -> ASCII

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/scanner.dart:58: * [:asciiString(0,-1):]
creates an ascii SourceString whose content is found
Ditto.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
File lib/compiler/implementation/scanner/token.dart (right):

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/token.dart:106: *  [stringValue] should not
be used outside the scanner/parser.
That is not entirely correct. stringValue should only be used for testing
keywords and symbols. An identifier, string, number, etc will return null. But
as a rule of thumb, use slowToString() instead.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/token.dart:127: String get text() =>
slowToString();
I'd prefer to use "slowToString()". As it has "slow" in its name.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/token.dart:132: int get charLength() =>
text.length;
This is slow.

http://codereview.chromium.org/10539021/diff/8016/tests/compiler/dart2js/scan...
File tests/compiler/dart2js/scanner_offset_length_test.dart (right):

http://codereview.chromium.org/10539021/diff/8016/tests/compiler/dart2js/scan...
tests/compiler/dart2js/scanner_offset_length_test.dart:39: check('foo'); //
identifier
Comments aren't proper sentences.

[Johnni Winther, 2012-06-25 10:40:00]

PTAL

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/parser.dart:1289: var tokenKind =
token.kind;
On 2012/06/25 08:12:37, ahe wrote:
> In other places I have used:
> 
> var kind = token.kind;

Done.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/parser.dart:1296: token =
parseStringPart(token);
On 2012/06/25 08:12:37, ahe wrote:
> The last two lines can be shared.

Done.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
File lib/compiler/implementation/scanner/scanner.dart (right):

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/scanner.dart:19: * characters whereas
[ByteArrayScanner] operators on byte arrays and thus
On 2012/06/25 08:12:37, ahe wrote:
> ... characters (Unicode codepoints represented as int) ...

Done.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/scanner.dart:28: * and content is determined
by [no] is appended. */
On 2012/06/25 08:12:37, ahe wrote:
> */ on its own line.

Done.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/scanner.dart:55: * Creates an ascii
SourceString whose content begins at the source byte
On 2012/06/25 08:12:37, ahe wrote:
> ascii -> ASCII

Done.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/scanner.dart:58: * [:asciiString(0,-1):]
creates an ascii SourceString whose content is found
On 2012/06/25 08:12:37, ahe wrote:
> Ditto.

Done.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
File lib/compiler/implementation/scanner/token.dart (right):

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/token.dart:106: *  [stringValue] should not
be used outside the scanner/parser.
On 2012/06/25 08:12:37, ahe wrote:
> That is not entirely correct. stringValue should only be used for testing
> keywords and symbols. An identifier, string, number, etc will return null. But
> as a rule of thumb, use slowToString() instead.

Done.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/token.dart:127: String get text() =>
slowToString();
On 2012/06/25 08:12:37, ahe wrote:
> I'd prefer to use "slowToString()". As it has "slow" in its name.

Done.

http://codereview.chromium.org/10539021/diff/8016/lib/compiler/implementation...
lib/compiler/implementation/scanner/token.dart:132: int get charLength() =>
text.length;
On 2012/06/25 08:12:37, ahe wrote:
> This is slow.

Done.

[ahe, 2012-06-25 10:55:52]

LGTM!

http://codereview.chromium.org/10539021/diff/22002/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10539021/diff/22002/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1308: // Parsing the
infix/suffix, for instance y and z' of 'x${id}y${id}z'
Very nice.

http://codereview.chromium.org/10539021/diff/22002/tests/compiler/dart2js/sca...
File tests/compiler/dart2js/scanner_offset_length_test.dart (right):

http://codereview.chromium.org/10539021/diff/22002/tests/compiler/dart2js/sca...
tests/compiler/dart2js/scanner_offset_length_test.dart:39: check('foo'); //
identifier
Comments still aren't proper sentences.