-
Notifications
You must be signed in to change notification settings - Fork 38
Remove debug formatting for CTLexerBuilder src gen #494
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
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,3 +44,4 @@ sparsevec = "0.2" | |
| static_assertions = "1.1" | ||
| unicode-width = "0.1.11" | ||
| vob = ">=3.0.2" | ||
| proc-macro2 = "1.0" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
To me this here, and the related bounds immediately below this are the most controversial aspects to this patch,
and the part that we need to consider most carefully.
The other thing we could do here is change this
ToTokenstoDisplay,Changing it to
ToTokenskind of just enables one minor cleanup, theQuoteOption(tok_id)cleanup in 18f1d9dI think it would definitely be fine to consider using
Displayhere instead ofToTokensand dropping that last cleanup?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.
One more thing to note here, which I had mentioned in a previous comment on usize,
using
ToTokenshere also has the effect of including the type in the value e.g. this patch is currently encodingStorageTvalues asSome (5u32)), a change fromSome(5)withDebugandDisplay.Looking at the sources generated by the testsuite these
StorageTvalues i'm seeing are all usingu32.So if we do go with
ToTokensit would be good to try and exercise this in the testsuite in a way that uses another type hereu8.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 don't have a very strong opinion here, but I'm somewhat leaning towards liking the
1_u8form ofToTokens.I tested the codegen of
ToTokenshere, by changing thelrpar/examples/calc_actions/build.rsto useCTLexerBuilder::<DefaultLexerTypes<u8>>::new_with_lexemet()and it makes sense to me that in theory it should be fine forStorageTto include the concrete type in literals, since kind of the point is that it has a concrete size known to the caller.Other places where we rely on the can be converted into a usize losslessly we could potentially use a newtype around known integer types to print literals without the type suffix.
Either way, since we didn't remove the
Debugsuffix, at worst we can go back to usingDebug?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'm fine with having the type suffix for literals.
One thing I haven't fully understood is: what types is
ToTokensimplemented on? I assume it's implemented on all primitive integers byquote? In that sense, this isn't going to be much of a breaking change unless a user does something odd like pass a newtype here: that would force them to know about theToTokenstrait andquote. In that sense,Displaywould be preferable, but I take your point in 18f1d9d thatDisplaymeans we end up having to do our own string escaping, which is a bit nasty. On that basis, I'm fine usingToTokens.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.
It's basically all the concete value types... bool, numerical, strings, option + reference types (minus Arc), some macro specific types that represent tokens.
It's sort of just a hodgepodge of various types, I don't see any reason why some things seem to be missing (like tuples, Arc)
The full list is here:
https://docs.rs/quote/latest/quote/trait.ToTokens.html#foreign-impls
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.
So long as the integer types are there, I think we'll have covered nearly every sensible use that I can conceive of.
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.
The one thing I guess I would say is that for the specific case of
StorageTwe don't have to worry about string escaping at all, I think it is also probably the most stable of all format printing we could likely expect format printing to be. So I don't think it is too far fetched to useToTokensinternally, but not add the bounds forStorageTand useDebugorDisplayjust for that.So to me
ToTokensoverDebugorDisplayforStorageTmostly revolves around whether we want to be able to include the type suffix in integer literals. It isn't a huge thing but maybe adds a little bit of type checking we don't currently get (for better or worse!).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.
If our plan is to use
quote(and friends) for all code generation inside grmtools, I think theToTokensbound makes sense: we'll have bought into that ecosystem, and might as well take advantage of it, and make clear that we've done so to users. If we don't plan on going on all the way withquotethen perhaps we should just fall back onDisplay.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.
Makes sense, I'd probably stick with the
ToTokensbound then since it seems like using it throughout will allow us to take advantage of it more heavily, by implementingToTokensfor things that contain aStorageT.Otherwise we're kind of doomed to a mixture of the quote ecosystem plus manually formatted values.