Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1387893002:
New lexer for mojom written in go. (Closed)
|
Created:
5 years, 2 months ago by azani Modified:
5 years, 2 months ago CC:
mojo-reviews_chromium.org, gregsimon, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://github.com/domokit/mojo.git@master Target Ref:
refs/heads/master Project:
mojo Visibility:
Public. |
DescriptionNew lexer for mojom written in go.
BUG=fixes #464
R=mattr@google.com, rudominer@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/600c05201d29661317b2c7991cb2af1da9695e57
Patch Set 1 #
Total comments: 28
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 46
Patch Set 5 : #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Messages
Total messages: 16 (1 generated)
azani@chromium.org changed reviewers: + mattr@google.com, rudominer@chromium.org
ptal
General remark: I would like to see a lot more comments. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... File mojom/mojom_parser/lexer/lexer.go (right): https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:6: ) Comment at top of file please. As part of this I suggest you give a high-level overview of the algorithm you are using. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:8: func Tokenize(source string) TokenStream { function comment please. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:19: lineOffset int comment each of these fields please. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:30: func (l *lexer) emitToken(tokenType TokenKind) { function comment https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:40: func (l *lexer) startToken() { function comment https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... File mojom/mojom_parser/lexer/lexer_test.go (right): https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer_test.go:19: func TestAllSingleTokens(t *testing.T) { Comment: What does this test test? https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... File mojom/mojom_parser/lexer/token_stream.go (right): https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/token_stream.go:7: type TokenStream interface { Please add a comment describing the role this interface plays in the larger scheme of the parser/lexer. I'm not sure what standard Go style has to say about this but I personally like it when each file has a short comment at the top telling me what the purpose of the file is and what I should expect to find in the file. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/token_stream.go:18: eof := Token{Kind: EOF} This could be a global variable instead of a function. Also as a function you could write just: return Token{Kind: EOF}. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/token_stream.go:22: // *TokenChan implements TokenStream. Please use a few more words for this comment. Describe how a Go channel is being used. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/token_stream.go:26: buffered bool comment for buffered. Perhaps use a different word than buffered since Go uses the terminology of a buffered channel and that is not what the word means here. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... File mojom/mojom_parser/lexer/tokens.go (right): https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/tokens.go:9: ) Comment at top of file please. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/tokens.go:17: ERROR_ILLEGAL_CHAR Add comments for each of the errors. Explain for example the difference between UNKNOWN and ILLEGAL_CHAR. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/tokens.go:63: // TODO(azani): Check that all tokens were implemented. Did you check? https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/tokens.go:158: // Note(rudominer) It is important to use %d below so as to avoid I know official Go style says that there is no limit to line lengths. Do you know if Google Go style says the same?
ptal I've addressed your comments. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... File mojom/mojom_parser/lexer/lexer.go (right): https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:6: ) On 2015/10/05 21:16:20, rudominer wrote: > Comment at top of file please. As part of this I suggest you give a high-level > overview of the algorithm you are using. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:8: func Tokenize(source string) TokenStream { On 2015/10/05 21:16:20, rudominer wrote: > function comment please. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:19: lineOffset int On 2015/10/05 21:16:20, rudominer wrote: > comment each of these fields please. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:30: func (l *lexer) emitToken(tokenType TokenKind) { On 2015/10/05 21:16:20, rudominer wrote: > function comment Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer.go:40: func (l *lexer) startToken() { On 2015/10/05 21:16:20, rudominer wrote: > function comment Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... File mojom/mojom_parser/lexer/lexer_test.go (right): https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/le... mojom/mojom_parser/lexer/lexer_test.go:19: func TestAllSingleTokens(t *testing.T) { On 2015/10/05 21:16:20, rudominer wrote: > Comment: What does this test test? Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... File mojom/mojom_parser/lexer/token_stream.go (right): https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/token_stream.go:7: type TokenStream interface { On 2015/10/05 21:16:20, rudominer wrote: > Please add a comment describing the role this interface plays in the larger > scheme of the parser/lexer. > > I'm not sure what standard Go style has to say about this but I personally like > it when each file has a short comment at the top telling me what the purpose of > the file is and what I should expect to find in the file. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/token_stream.go:18: eof := Token{Kind: EOF} On 2015/10/05 21:16:20, rudominer wrote: > This could be a global variable instead of a function. > Also as a function you could write just: > return Token{Kind: EOF}. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/token_stream.go:22: // *TokenChan implements TokenStream. On 2015/10/05 21:16:20, rudominer wrote: > Please use a few more words for this comment. Describe how a Go channel is being > used. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/token_stream.go:26: buffered bool On 2015/10/05 21:16:20, rudominer wrote: > comment for buffered. Perhaps use a different word than buffered since Go uses > the terminology of a buffered channel and that is not what the word means here. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... File mojom/mojom_parser/lexer/tokens.go (right): https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/tokens.go:9: ) On 2015/10/05 21:16:20, rudominer wrote: > Comment at top of file please. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/tokens.go:17: ERROR_ILLEGAL_CHAR On 2015/10/05 21:16:20, rudominer wrote: > Add comments for each of the errors. Explain for example the difference between > UNKNOWN and ILLEGAL_CHAR. Done. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/tokens.go:63: // TODO(azani): Check that all tokens were implemented. On 2015/10/05 21:16:21, rudominer wrote: > Did you check? Yes. https://codereview.chromium.org/1387893002/diff/1/mojom/mojom_parser/lexer/to... mojom/mojom_parser/lexer/tokens.go:158: // Note(rudominer) It is important to use %d below so as to avoid On 2015/10/05 21:16:21, rudominer wrote: > I know official Go style says that there is no limit to line lengths. Do you > know if Google Go style says the same? AFAICT, Google's Go style guide is the official go style guide.
Friendly Ping. Hey Matt, do you think you will be able to take a look?
Actually the CL looks great! I made a few nitpicky comments. Although some comments applied to multiple places, I only marked them once. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/lexer.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:92: width = 1 This struck me as strange, this means the input was empty, I would think this would lead you to a out-of-bounds exception next time this is called. I'm probably missing something. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/tokens.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/tokens.go:19: ERROR_UNKNOWN TokenKind = iota The convention in go is to use camel case even for constants: ErrorUnknown. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/tokens.go:177: // This method is used to generate user-facing strings in compilation error The usual Go convention is to start all comments with the name of the thing being commented: // ShortLocationString is used to generate user-facing strings in ...
https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/lexer.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:5: // To call the lexer, call Tokenize with the source string to obtain To use the lexer, call... https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:28: // Tokenize accepts a source string and returns the tokens which compose The phrasing belies the asynchronous nature. Also do tokens "comprise" a source string? Not literally in the sense that the source string is not the concatenation of the tokens. How about: "Tokenize accepts a source string and parses it into a stream of tokens. It returns a TokenStream from which the caller may read the tokens." https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:38: // source is the source code to be lexed. nit: You don't need to repeat the variable name with the word "is". This adds no information. I know that "Effective Go" talks about starting godoc comments with the name of the item being commented. But I'm not sure it means to include the case of private member variables. There are examples in Effect Go itself of not doing this. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:41: // offset is the number of characters that have been consumed. Let's follow the go standard terminology and avoid the term character and use rune and byte. Is this a number of bytes or a number of runes? That same issue arises with all of your variables. Are we counting runes or bytes. I think for any number that translates into an end-user viewable message such as the column number of a token, you will want to use runes and not bytes. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:49: lineOffset int I think it would be clearer if we didn't over use the word offset. Lets pick offset to mean either the position in the source text from the beginning or the position from the beginning of the line and use a different word for the other thing. For example we could use column for the position within a line. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:51: // start is the index in source where the current token begins. Maybe currentTokenStartIndex or currentTokenStart? https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:55: lineStart int Maybe currentTokenLineNumber? https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:59: lineOffsetStart int Maybe currentTokenColumn? https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:61: // tokens is a channel on which the found tokens are emitted. maybe "to which the found..." https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:70: // emitToken emits the current token and begins the new token. begins a new token https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:81: // startToken starts the new token. starts a new token https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:91: if width == 0 { What is your thinking here? Can width=0 without c being equal to RuneError? Shouldn't you be checking for c=RuneError here? https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:106: char, _ := utf8.DecodeRuneInString(l.source[l.offset:]) What if char == RuneError? https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:113: return l.offset >= len(l.source) Note that this depends on l.offset being a number of bytes and not a number of runes. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:118: // This is a state machine. "This" is ambiguous here. Maybe "We are implementing a state machine." https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:132: // This is the beginning state and also the state in between tokens. Nit. In my understanding of a DFA there is no "state in between tokens". A DFA has a set of states and a set of transitions between states and each transition consumes some number of input tokens. If that is the mental model you have also maybe you could re-phrase this comment. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:134: if l.offset >= len(l.source) { You have a function IsEos() for this, no? https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/lexer_test.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer_test.go:14: Can you add a test where - the input string is empty - the input string contains bytes that are not valid UTF8 https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/token_stream.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/token_stream.go:20: // An EOF token is returned by TokenStream to signal the end of the stream. The EOF token? https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/token_stream.go:53: // *TokenSlice implements TokenStream I'm not sure anything is going to use TokenSlice. Maybe we should get rid of it.
Notice that https://codereview.chromium.org/1384243002/ has landed and this includes a stub for mojom/mojom_parser/lexer/lexer_test.go. When you get a chance please pull in that CL so that you will be running your test on the trybots.
Hey Matt, can you take a look at the comment below and let us know? https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/lexer.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:38: // source is the source code to be lexed. On 2015/10/12 18:06:05, rudominer wrote: > nit: You don't need to repeat the variable name with the word "is". This adds no > information. I know that "Effective Go" talks about starting godoc comments with > the name of the item being commented. But I'm not sure it means to include the > case of private member variables. There are examples in Effect Go itself of not > doing this. Hey Matt, could you settle that question for us? I'm happy either way.
I finished my review and after addressing my previous batch of comments I will be happy. Probably we will want some more edge-case testing but I think we should proceed with the integration into the pipeline because that will give us lots of natural test cases that will naturally find bugs for us. We can track any further refinements of this as bugs.
ptal https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/lexer.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:5: // To call the lexer, call Tokenize with the source string to obtain On 2015/10/12 18:06:05, rudominer wrote: > To use the lexer, call... Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:28: // Tokenize accepts a source string and returns the tokens which compose On 2015/10/12 18:06:05, rudominer wrote: > The phrasing belies the asynchronous nature. Also do tokens "comprise" a source > string? Not literally in the sense that the source string is not the > concatenation of the tokens. How about: "Tokenize accepts a source string and > parses it into a stream of tokens. It returns a TokenStream from which the > caller may read the tokens." Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:41: // offset is the number of characters that have been consumed. On 2015/10/12 18:06:05, rudominer wrote: > Let's follow the go standard terminology and avoid the term character and use > rune and byte. Is this a number of bytes or a number of runes? That same issue > arises with all of your variables. Are we counting runes or bytes. I think for > any number that translates into an end-user viewable message such as the column > number of a token, you will want to use runes and not bytes. Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:49: lineOffset int On 2015/10/12 18:06:05, rudominer wrote: > I think it would be clearer if we didn't over use the word offset. Lets pick > offset to mean either the position in the source text from the beginning or the > position from the beginning of the line and use a different word for the other > thing. For example we could use column for the position within a line. Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:51: // start is the index in source where the current token begins. On 2015/10/12 18:06:05, rudominer wrote: > Maybe currentTokenStartIndex > or currentTokenStart? Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:55: lineStart int On 2015/10/12 18:06:05, rudominer wrote: > Maybe currentTokenLineNumber? Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:59: lineOffsetStart int On 2015/10/12 18:06:05, rudominer wrote: > Maybe currentTokenColumn? Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:61: // tokens is a channel on which the found tokens are emitted. On 2015/10/12 18:06:05, rudominer wrote: > maybe "to which the found..." Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:70: // emitToken emits the current token and begins the new token. On 2015/10/12 18:06:05, rudominer wrote: > begins a new token Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:81: // startToken starts the new token. On 2015/10/12 18:06:05, rudominer wrote: > starts a new token Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:91: if width == 0 { On 2015/10/12 18:06:05, rudominer wrote: > What is your thinking here? Can width=0 without c being equal to RuneError? > Shouldn't you be checking for c=RuneError here? RuneError is just a special rune. It is handled the same way as any other illegal character. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:92: width = 1 On 2015/10/12 18:04:13, mattr wrote: > This struck me as strange, this means the input was empty, I would think this > would lead you to a out-of-bounds exception next time this is called. I'm > probably missing something. Basically, in all cases where this branch is taken (AFAICT), the caller will check IsEos and exit the main loop. But I added a check here for robustness-sake. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:106: char, _ := utf8.DecodeRuneInString(l.source[l.offset:]) On 2015/10/12 18:06:05, rudominer wrote: > What if char == RuneError? We treat it like any other rune. It looks like RuneError is actually the "unicode replacement character". https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:113: return l.offset >= len(l.source) On 2015/10/12 18:06:05, rudominer wrote: > Note that this depends on l.offset being a number of bytes and not a number of > runes. Yes, since strings index based on bytes, not runes. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:118: // This is a state machine. On 2015/10/12 18:06:06, rudominer wrote: > "This" is ambiguous here. Maybe "We are implementing a state machine." Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:132: // This is the beginning state and also the state in between tokens. On 2015/10/12 18:06:05, rudominer wrote: > Nit. In my understanding of a DFA there is no "state in between tokens". A DFA > has a set of states and a set of transitions between states and each transition > consumes some number of input tokens. If that is the mental model you have also > maybe you could re-phrase this comment. I meant returned to after most tokens are emitted. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer.go:134: if l.offset >= len(l.source) { On 2015/10/12 18:06:05, rudominer wrote: > You have a function IsEos() for this, no? Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/lexer_test.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/lexer_test.go:14: On 2015/10/12 18:06:06, rudominer wrote: > Can you add a test where > - the input string is empty > - the input string contains bytes that are not valid UTF8 Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/token_stream.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/token_stream.go:20: // An EOF token is returned by TokenStream to signal the end of the stream. On 2015/10/12 18:06:06, rudominer wrote: > The EOF token? Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/token_stream.go:53: // *TokenSlice implements TokenStream On 2015/10/12 18:06:06, rudominer wrote: > I'm not sure anything is going to use TokenSlice. Maybe we should get rid of it. Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... File mojom/mojom_parser/lexer/tokens.go (right): https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/tokens.go:19: ERROR_UNKNOWN TokenKind = iota On 2015/10/12 18:04:13, mattr wrote: > The convention in go is to use camel case even for constants: ErrorUnknown. Done. https://codereview.chromium.org/1387893002/diff/60001/mojom/mojom_parser/lexe... mojom/mojom_parser/lexer/tokens.go:177: // This method is used to generate user-facing strings in compilation error On 2015/10/12 18:04:13, mattr wrote: > The usual Go convention is to start all comments with the name of the thing > being commented: > > // ShortLocationString is used to generate user-facing strings in ... Done.
lgtm
On 2015/10/13 17:34:31, rudominer wrote: > lgtm Hey Matt, do you want to take another look and lgtm?
I added one question, but LGTM. https://codereview.chromium.org/1387893002/diff/100001/mojom/mojom_parser/lex... File mojom/mojom_parser/lexer/lexer.go (right): https://codereview.chromium.org/1387893002/diff/100001/mojom/mojom_parser/lex... mojom/mojom_parser/lexer/lexer.go:104: if width == 0 { I guess my question was, why increment width here at all? It doesn't seem to serve any purpose.
https://codereview.chromium.org/1387893002/diff/100001/mojom/mojom_parser/lex... File mojom/mojom_parser/lexer/lexer.go (right): https://codereview.chromium.org/1387893002/diff/100001/mojom/mojom_parser/lex... mojom/mojom_parser/lexer/lexer.go:104: if width == 0 { On 2015/10/14 19:29:26, mattr wrote: > I guess my question was, why increment width here at all? It doesn't seem to > serve any purpose. It turns out it's not necessary... Gone now and thanks for pushing so I could figure this out.
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as 600c05201d29661317b2c7991cb2af1da9695e57 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
