Add HasField::project; simplify is_bit_valid
#2843
Conversation
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 introduces a new 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a project method to the HasField trait, which is then used to significantly simplify the implementation of is_bit_valid in derive macros by removing duplicated code. The changes are well-structured and improve the maintainability of the codebase. I have one minor suggestion to correct a safety comment to ensure it accurately reflects the code it's documenting.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2843 +/- ##
==========================================
- Coverage 92.05% 91.94% -0.11%
==========================================
Files 20 20
Lines 5812 5824 +12
==========================================
+ Hits 5350 5355 +5
- Misses 462 469 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jswrenn
force-pushed
the
G9e7039c715e1ec53e6d860909bb0d618898fd46b
branch
3 times, most recently
from
3574dd4 to
ba063c7
Compare
|
Although I've omitted it from this PR, the building blocks are here for ergonomic projection: #[derive(TryFromBytes)]
struct Foo {
pub a: Bar,
pub(crate) b: u16,
c: u32,
}
#[derive(TryFromBytes, Immutable)]
union Bar {
pub a: u8,
pub(crate) b: Baz,
c: u32,
}
#[derive(TryFromBytes, Immutable, Clone, Copy)]
#[repr(C)]
enum Baz {
A(u8, u16),
B { x: u8, y: u16 },
C,
}
macro_rules! project {
($p:tt . $first_field:tt $(. $rest_fields:tt)+) => {{
let field = project!($p . $first_field);
project!(field $(. $rest_fields)*)
}};
($p:tt . $field:ident) => {
zerocopy::HasField::<_, _, { zerocopy::ident_id!($field) }>::project($p)
};
($p:tt . ($variant:ident :: $field:ident)) => {
zerocopy::HasField::<_, { zerocopy::ident_id!($variant) }, { zerocopy::ident_id!($field) }>::project($p)
};
}
fn foo(foo: PtrInner<'_, Foo>) -> PtrInner<'_, u8> {
project!(foo.a.b.x)
}For more breadcrumbs, see #196. |
joshlf
left a comment
joshlf
left a comment
There was a problem hiding this comment.
Lots of duplication of logic and safety comments here. Reasonable for now, but makes me wonder how much deduplication we could do here (and elsewhere in zerocopy). Specifically, regarding the invariants on PtrInner.
jswrenn
force-pushed
the
G9e7039c715e1ec53e6d860909bb0d618898fd46b
branch
from
ba063c7 to
a524fca
Compare
jswrenn
force-pushed
the
G2238bc341570838db412d880017b3f0c25ac09fa
branch
2 times, most recently
from
d8e3bf8 to
bce4eeb
Compare
jswrenn
force-pushed
the
G9e7039c715e1ec53e6d860909bb0d618898fd46b
branch
from
a524fca to
33f3fb6
Compare
jswrenn
force-pushed
the
G9e7039c715e1ec53e6d860909bb0d618898fd46b
branch
2 times, most recently
from
345b7d3 to
abf9b1c
Compare
joshlf
left a comment
joshlf
left a comment
There was a problem hiding this comment.
Ooops, didn't notice failing tests.
jswrenn
force-pushed
the
G9e7039c715e1ec53e6d860909bb0d618898fd46b
branch
2 times, most recently
from
dcf49b9 to
4e3e874
Compare
jswrenn
force-pushed
the
G9e7039c715e1ec53e6d860909bb0d618898fd46b
branch
from
4e3e874 to
898e717
Compare
joshlf
force-pushed
the
G9e7039c715e1ec53e6d860909bb0d618898fd46b
branch
from
898e717 to
e28215f
Compare
This method projects from `PtrInner<_, Self>` to a `PtrInner` of a field of `Self`. In this commit, we use `HasField::project` to considerably simplify `is_bit_valid` implementations. gherrit-pr-id: G9e7039c715e1ec53e6d860909bb0d618898fd46b
joshlf
force-pushed
the
G9e7039c715e1ec53e6d860909bb0d618898fd46b
branch
from
e28215f to
122da00
Compare
3 participants
This method projects from
PtrInner<_, Self>to aPtrInnerof a fieldof
Self. In this commit, we useHasField::projectto considerablysimplify
is_bit_validimplementations.HasField::project; simplifyis_bit_valid#2843Latest Update: v19 — Compare vs v18
📚 Full Patch History
Links show the diff between the row version and the column version.