-
Notifications
You must be signed in to change notification settings - Fork 136
Introduce code generation mechanism #2844
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
base: G9e7039c715e1ec53e6d860909bb0d618898fd46b
Are you sure you want to change the base?
Introduce code generation mechanism #2844
Conversation
gherrit-pr-id: Gae2a2cdb24c00b662f0909211910d1afe272d793
Summary of ChangesHello @jswrenn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a code generation framework within the project, specifically to automate the creation of boilerplate trait implementations. The primary application demonstrated is the automatic derivation of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a code generation mechanism to implement Immutable for tuples, which is a great addition. The implementation is cleverly done by refactoring ImplBlockBuilder to be more generic and reusing it within a test function that generates the code. The refactoring across zerocopy-derive to use the new ImplBlockBuilder::from_derive_input constructor and consolidate logic for structs, enums, and unions is a significant improvement, making the code cleaner and more maintainable.
I have one suggestion to improve the developer experience around the code generation test. Otherwise, the changes look solid and well-executed.
| fn codegen() { | ||
| let path = { | ||
| use std::path::PathBuf; | ||
| let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); | ||
| assert!(path.pop()); | ||
| path.push("src"); | ||
| path.push("generated.rs"); | ||
| path | ||
| }; | ||
|
|
||
| let generated = [HEADER, &tuples()].map(ToString::to_string).join("\n"); | ||
|
|
||
| std::fs::write(path, generated.as_bytes()).unwrap(); | ||
| } |
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 current implementation of codegen as a test has the side effect of modifying the source tree whenever cargo test is run. This can be surprising for developers and lead to a dirty worktree.
A more robust pattern is to make the test verify that the generated code is up-to-date, and only update it when an environment variable is set. This makes cargo test a verification step, and updating the code an explicit action. This also improves CI, as it can fail if generated code is not checked in.
I've also replaced unwrap() with expect() to provide more helpful error messages.
fn codegen() {
let path = {
use std::path::PathBuf;
let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
assert!(path.pop());
path.push("src");
path.push("generated.rs");
path
};
let generated = [HEADER, &tuples()].map(ToString::to_string).join("\n");
if std::env::var("ZEROCOPY_UPDATE_CODEGEN").is_ok() {
std::fs::write(&path, generated.as_bytes())
.expect("failed to write generated code to src/generated.rs");
} else {
let existing = std::fs::read_to_string(&path)
.expect("failed to read existing generated code from src/generated.rs");
if existing != generated {
panic!("\n\nGenerated code is out of date. Run `ZEROCOPY_UPDATE_CODEGEN=1 cargo test` to update.\n\n");
}
}
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## G9e7039c715e1ec53e6d860909bb0d618898fd46b #2844 +/- ##
==========================================================================
Coverage 92.05% 92.05%
==========================================================================
Files 20 20
Lines 5812 5812
==========================================================================
Hits 5350 5350
Misses 462 462 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR is on branch fields.
HasField::project; simplifyis_bit_valid#2843HasFieldtrait #2842