Update the mojom lexer to emit comment tokens.

Update the mojom lexer to emit comment tokens.

Also, add a filtered TokenStream so we can prevent the parser from seeing this
tokens until it can handle them.

R=rudominer@chromium.org, rudominer
BUG= #592

Committed: https://chromium.googlesource.com/external/mojo/+/25d53bf53b4040f29db984d4774483c5de9d2dc5

[azani, 2016-01-16 00:07:12]

ptal

Sorry for the premature send.

[rudominer, 2016-01-16 01:08:58]

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
File mojom/mojom_parser/lexer/lexer.go (right):

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/lexer.go:29: // can be read from the returned
TokenStream.
You should comment that this function does not return comments.

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/lexer.go:38: // tokenizeUnfiltered does not filter any
token.
I would add a slightly longer comment that explains the role this function has
relative to the function Tokenize.

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
File mojom/mojom_parser/lexer/lexer_test.go (right):

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/lexer_test.go:191: // TestSingleLineComment tests that
single line comments are correctly lexed.
Don't you want to test both that they are skipped when they should be and not
skipped otherwise?

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/lexer_test.go:202: // TestMultiLineComment tests that
multi line comments are correctly skipped.
skipped?

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
File mojom/mojom_parser/lexer/token_stream.go (right):

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/token_stream.go:67: for t := s.tokenStream.PeekNext();
s.isBlacklisted(t); s.tokenStream.ConsumeNext() {
Shouldn't there be some condition here for end-of-stream? Can you add a test
with a stream that contains nothing but blacklisted tokens.

[rudominer, 2016-01-19 17:44:24]

lgtm

lgtm with comment comments.

[azani, 2016-01-20 00:00:11]

ptal I made some changes that I would like a second set of eyes on.

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
File mojom/mojom_parser/lexer/lexer.go (right):

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/lexer.go:29: // can be read from the returned
TokenStream.
On 2016/01/16 01:08:58, rudominer wrote:
> You should comment that this function does not return comments.

Done.

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/lexer.go:38: // tokenizeUnfiltered does not filter any
token.
On 2016/01/16 01:08:58, rudominer wrote:
> I would add a slightly longer comment that explains the role this function has
> relative to the function Tokenize.

Done.

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
File mojom/mojom_parser/lexer/lexer_test.go (right):

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/lexer_test.go:191: // TestSingleLineComment tests that
single line comments are correctly lexed.
On 2016/01/16 01:08:58, rudominer wrote:
> Don't you want to test both that they are skipped when they should be and not
> skipped otherwise?

Done.

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/lexer_test.go:202: // TestMultiLineComment tests that
multi line comments are correctly skipped.
On 2016/01/16 01:08:58, rudominer wrote:
> skipped?

Done.

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
File mojom/mojom_parser/lexer/token_stream.go (right):

https://codereview.chromium.org/1593543004/diff/60001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/token_stream.go:67: for t := s.tokenStream.PeekNext();
s.isBlacklisted(t); s.tokenStream.ConsumeNext() {
On 2016/01/16 01:08:58, rudominer wrote:
> Shouldn't there be some condition here for end-of-stream? Can you add a test
> with a stream that contains nothing but blacklisted tokens.

No. isBlacklisted always returns false if the underlying stream returns EOF. But
I was using the for loop wrong, so I've restructured it to make it clearer.

[rudominer, 2016-01-20 00:40:31]

https://codereview.chromium.org/1593543004/diff/80001/mojom/mojom_parser/lexe...
File mojom/mojom_parser/lexer/token_stream.go (right):

https://codereview.chromium.org/1593543004/diff/80001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/token_stream.go:78: s.tokenStream.ConsumeNext()
I wonder if there should be a ConsumeNextInternal() that is invoked from
PeekNext() and then ConsumeNext() should be implemented as

{
  s.PeekNext()
  s.ConsumeNextInternal()
}

As it is now, if I invoke ConsumeNext() without invoking PeekNext() then it may
not actually consume a (non-filtered) token.

If you do change it as I suggest, then please add an additional test that would
have failed with the current version.

https://codereview.chromium.org/1593543004/diff/80001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/token_stream.go:84: if token.Kind == EOF {
Instead of this, make a constructor for FilteredTokenStream and check there that
EOF is not blacklisted.

https://codereview.chromium.org/1593543004/diff/80001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/token_stream.go:87: _, ok := s.blacklist[token.Kind]
After the above change you can make this function one line.

[azani, 2016-01-25 20:37:06]

https://codereview.chromium.org/1593543004/diff/80001/mojom/mojom_parser/lexe...
File mojom/mojom_parser/lexer/token_stream.go (right):

https://codereview.chromium.org/1593543004/diff/80001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/token_stream.go:78: s.tokenStream.ConsumeNext()
On 2016/01/20 00:40:31, rudominer wrote:
> I wonder if there should be a ConsumeNextInternal() that is invoked from
> PeekNext() and then ConsumeNext() should be implemented as
> 
> {
>   s.PeekNext()
>   s.ConsumeNextInternal()
> }
> 
> As it is now, if I invoke ConsumeNext() without invoking PeekNext() then it
may
> not actually consume a (non-filtered) token.
> 
> If you do change it as I suggest, then please add an additional test that
would
> have failed with the current version.

Done.

https://codereview.chromium.org/1593543004/diff/80001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/token_stream.go:84: if token.Kind == EOF {
On 2016/01/20 00:40:31, rudominer wrote:
> Instead of this, make a constructor for FilteredTokenStream and check there
that
> EOF is not blacklisted.

Done.

https://codereview.chromium.org/1593543004/diff/80001/mojom/mojom_parser/lexe...
mojom/mojom_parser/lexer/token_stream.go:87: _, ok := s.blacklist[token.Kind]
On 2016/01/20 00:40:31, rudominer wrote:
> After the above change you can make this function one line.

Done.

[rudominer, 2016-01-25 20:47:41]

lgtm with one nit: I think the phrase "blacklisted" has the wrong connotation
here. I would use "filtered" or "ignored".

[azani, 2016-01-25 21:49:10]

Description was changed from

==========
Update the mojom lexer to emit comment tokens.

Also, add a filtered TokenStream so we can prevent the parser from seeing this
tokens until it can handle them.

R=rudominer
BUG= #592
==========

to

==========
Update the mojom lexer to emit comment tokens.

Also, add a filtered TokenStream so we can prevent the parser from seeing this
tokens until it can handle them.

R=rudominer@chromium.org, rudominer
BUG= #592

Committed:
https://chromium.googlesource.com/external/mojo/+/25d53bf53b4040f29db984d4774...
==========

[azani, 2016-01-25 21:49:11]

Committed patchset #8 (id:140001) manually as
25d53bf53b4040f29db984d4774483c5de9d2dc5 (presubmit successful).