Generate trait implementations for tuples #2862
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 significantly extends the utility of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 trait implementations for tuples up to size 26, which is a valuable addition. The implementation uses a complex but well-structured macro. The refactoring of is_bit_valid logic into a helper function is_field_valid is a good improvement for maintainability. I've found one critical issue in the derive macro that will cause compilation failures for users and needs to be addressed.
jswrenn
force-pushed
the
G992a50c1ccf344487e1cd764afe0b17e566bc620
branch
from
489515e to
96eb35c
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2862 +/- ##
==========================================
- Coverage 91.94% 91.80% -0.15%
==========================================
Files 20 20
Lines 5824 5843 +19
==========================================
+ Hits 5355 5364 +9
- Misses 469 479 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Emit implementations of `HasField`, `Immutable`, `TryFromBytes`, `FromZeros`, and `FromBytes` for tuples of length up to 26. Closes #274 gherrit-pr-id: G992a50c1ccf344487e1cd764afe0b17e566bc620
jswrenn
force-pushed
the
G992a50c1ccf344487e1cd764afe0b17e566bc620
branch
from
96eb35c to
96cb8ae
Compare
| // `STRUCT_VARIANT_ID`. | ||
| // - `$CurrI` is the field at index `$CurrI`, so `FIELD_ID` is | ||
| // `zerocopy::ident_id!($CurrI)` | ||
| // - `()` has the same visibility as `$CurrI` |
There was a problem hiding this comment.
| // - `()` has the same visibility as `$CurrI` | |
| // - `()` has the same visibility as the `.$CurrI` field (ie, `.0`, `.1`, etc) | |
| // since tuple fields are globally public, and so is the `()` type |
There was a problem hiding this comment.
I know it's technically fine to just say 0, 1, etc in the same way that it'd be fine for named fields to refer to them without the leading dot (foo, bar, etc), but at least to me, this makes it clearer that we're talking about the visibility of tuple fields, which is not something most people are used to doing.
| let slf = slf.as_non_null().as_ptr(); | ||
| // SAFETY: `PtrInner` promises it references either a zero-sized | ||
| // byte range, or else will reference a byte range that is | ||
| // entirely contained within an allocated object. In either | ||
| // case, this guarantees that `(*slf).$CurrI` is in-bounds of | ||
| // `slf`. | ||
| let field = unsafe { core::ptr::addr_of_mut!((*slf).$CurrI) }; | ||
|
|
||
| // SAFETY: `PtrInner` promises it references either a zero-sized | ||
| // byte range, or else will reference a byte range that is | ||
| // entirely contained within an allocated object. In either | ||
| // case, this guarantees that field projection will not wrap | ||
| // around the address space, and so `field` will be non-null. | ||
| let ptr = unsafe { core::ptr::NonNull::new_unchecked(field) }; | ||
|
|
||
| // SAFETY: | ||
| // 0. `ptr` addresses a subset of the bytes of | ||
| // `slf`, so by invariant on `slf: PtrInner`, | ||
| // if `ptr`'s referent is not zero sized, | ||
| // then `ptr` has valid provenance for its | ||
| // referent, which is entirely contained in | ||
| // some Rust allocation, `A`. | ||
| // 1. By invariant on `slf: PtrInner`, if | ||
| // `ptr`'s referent is not zero sized, `A` is | ||
| // guaranteed to live for at least `'a`. | ||
| unsafe { crate::PtrInner::new(ptr) } |
There was a problem hiding this comment.
TODO(me): I haven't reviewed this yet because I'm assuming we'll rebase on #2860, which will allow us to significantly simplify this body.
| <#field_tys as #zerocopy_crate::TryFromBytes>::is_bit_valid(field_candidate) | ||
| })* | ||
| true | ||
| #(&& #zerocopy_crate::util::macro_util::is_field_valid::<_, _, _, { #zerocopy_crate::ident_id!(#field_names) }>(candidate.reborrow()))* |
3 participants
Emit implementations of
HasField,Immutable,TryFromBytes,FromZeros, andFromBytesfor tuples of length up to 26.Closes #274
Latest Update: v3 — Compare vs v2
📚 Full Patch History
Links show the diff between the row version and the column version.