Implement a new lexer (and split Token and related code out of experimental/ast)
#358
Conversation
|
I would like to note that it is really nice to feel like I am "iterating" on existing code now XD |
mcy
force-pushed
the
mcy/lexer2
branch
2 times, most recently
from
7306a37
to
ca721ad
Compare
experimental/internal/with.go
Outdated
| // This is helpful for immediately panicking on function entry. | ||
| func PanicIfNil[C comparable](with *With[C]) { | ||
| if with.Nil() { | ||
| panic(fmt.Errorf("use of zero value: %p", with)) |
There was a problem hiding this comment.
Printing the address of with doesn't seem helpful for users that get this error. The message seems a little vague, too. Maybe instead:
| panic(fmt.Errorf("use of zero value: %p", with)) | |
| var zero C | |
| panic(fmt.Errorf("value has nil context %T", zero)) |
There was a problem hiding this comment.
Ooh, I always forget about %T.
experimental/parser/lexer.go
Outdated
| digits: | ||
| for i := 0; i < digits; i++ { | ||
| if l.Done() { | ||
| break escapeLoop |
There was a problem hiding this comment.
In general, let's not use labels -- this form of break is basically a goto. You could use a couple of local var flags to get the right control flow. But even better is likely to factor some of this out to a function, where you can use break vs return to control the level at which the loops are resumed. That would also help with readability since this function is kind of a monster.
There was a problem hiding this comment.
This particular label is gone.
Personally I am kind of allergic to using functions in lieu of labels. However, this function did benefit from some chopping up regardless.
experimental/parser/lexer.go
Outdated
| case r >= 'A' && r <= 'F': | ||
| value |= uint32(r) - 'A' + 10 | ||
| default: | ||
| break digits |
There was a problem hiding this comment.
This is not correct for \u and \U escapes, which must be exactly four or eight. Only \x can be short.
There was a problem hiding this comment.
Their length is validated elsewhere.
experimental/parser/lexer.go
Outdated
| }) | ||
| return | ||
| } | ||
| text = text[utf8.RuneLen(r):] |
There was a problem hiding this comment.
Will utf8.RuneLen always be correct? Just want to make sure whether it's possible for an encoder to marshal a rune in a "non-canonical" way, where we'd really need the original length from utf8.DecodeRune.
There was a problem hiding this comment.
UTF-8 does not permit over-long encodings, which DecodeRune rejects.
This PR implements a new Protobuf lexer under
experimental/parser. It is a fairly lenient lexer that parses many thingsprotocdoes not, and diagnoses them.Along with this I have separated the token parts of
experimental/astinto its own package. This was actually a really good idea, and makes the token code (arguably the fussiest part of the AST library) easier to follow. It's a good sign when three files become eight.I also did some work in
experimental/reportto make it so there is Only One True Span Type,report.Span. This side-steps a lot of the nastiness around carrying line and column information around; it is now truly only computed on-demand when diagnostics are rendered. Also, hooray for the massiveexperimental/reporttest suite.