Drop BigEndian from codegen input #81
Merged
Conversation
This file contains 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
rsheeter
force-pushed
the
no_be
branch
6 times, most recently
from
411239d to
e3cfb25
Compare
rsheeter
changed the title
rsheeter
force-pushed
the
no_be
branch
10 times, most recently
from
b6f266b to
6798c44
Compare
rsheeter
force-pushed
the
no_be
branch
3 times, most recently
from
3a0e908 to
40b0156
Compare
cmyr
reviewed
Oct 24, 2022
There was a problem hiding this comment.
This ended up being quite a bit cleaner than I expected, and the codegen inputs definitely make sense. I think a lot of the things I would otherwise touch on here were mentioned in our call, so I will try to be brief:
- on returning spanned errors versus panicking: this is definitely a tradeoff? One way to think about it is about whether you're using the tool with a new codegen input, or if you're working on the codegen code itself. If you're working on the codegen code, panciking is useful, but if you're working on a new codegen input this is much less true. Which case matters more? In my work I was optimizing for making it easier to use the codegen tool than to hack on it, so I made an effort to avoid panicking where possible, and focus on reporting errors in the input files.
BigEndian: if we don't have it here, we've lost one of the main motivations for having it in the code at all, so this is something to revisit in followup.- logging: it took me some googling to figure out what env var I should pass to enable logging, and when I did it seemed to produce a lot of noise. Maybe the solution here is to change the default level to
Info, and useInfofor things we want to print regardless?
Overall it's definitely encouraging to see you jump in here and make significant changes without too much evident frustration, I definitely wasn't sure how much joy folks would find in modifying the codegen code itself.
cmyr
approved these changes
Oct 24, 2022
|
Should the codegen tour's section on BigEndian be updated somehow after this PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Remove BigEndian from codegen inputs. Output is unchanged, except a few places where a (pointless?)
BigEndian<u8>is changed to nakedu8. Fixes #71.I have a lot of noisy logging that I would ideally like to keep but have toggled off by default, akin to a debug level log in normal Google source. Not sure how best to get that in Rust. EDIT: per @dfrg I added log & env_logger which seem to work well.