-
-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework TOML token parsing #100
Conversation
Now casting the current toml character to u32 for easier matching against the values provided in the abnf for toml. Removed the existing logic for parsing most characters to start again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks pretty nice, thanks for the PR. If we can find a more readable way to handle the matching ( without trashing codegen) I would prefer that
src/toml.rs
Outdated
#[allow(unreachable_patterns)] | ||
match self.cur as u32 { | ||
// , | ||
0x2C => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would really prefer if these stayed as char
s instead of converting to u32
first. It might make the codegen a tiny bit worse, but I really doubt it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, will do.
Sorry but I'm a bit confused - by codegen are you referring to generating the match statement from the ABNF?
Is this better? I would add comments with the characters next to each range in the macro, but many don't display in my system font. Can still do so if you feel it would help. |
No I think that looks great. I'll give it a day or so and see if @not-fl3 has any input, if not will merge soon. Thanks for fixing this up! |
No problem, thanks for the great project! |
LGTM! Honestly I just don't have much of an opinion on TOML, feel free to merge! |
Merging it is then. Thanks @lkirkwood |
This PR closes #81 and also allows a number of other valid identifiers to be parsed by reworking the logic for parsing TOML tokens from characters.
For example, all of the identifiers in the following document are valid according to the TOML spec (ABNF here), but none are currently able to be parsed:
(although even github markdown highlighting doesn't get that last one)