-
Notifications
You must be signed in to change notification settings - Fork 138
[WIP] #2864
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: Gb15e30e716a041285c1bf7434ce3163f594526a7
Are you sure you want to change the base?
[WIP] #2864
Conversation
Summary of ChangesHello @joshlf, 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 significant refactoring 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
|
ed42ba1 to
f2813bb
Compare
8338ac3 to
6fe302c
Compare
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 refactors the TryFromBytes::is_bit_valid method by removing its generic parameter for aliasing invariants. This simplifies the trait's API. The new implementation approach uses ReadOnly<T> to handle types with interior mutability like UnsafeCell, which seems like a sound refactoring.
My review has identified a few minor issues, mostly related to duplicated #[inline] attributes and some formatting inconsistencies in generated code within macros. These are likely leftovers from the refactoring and should be straightforward to fix.
| // is. Thus, this is a sound implementation of | ||
| // `UnsafeCell::is_bit_valid`. | ||
| T::is_bit_valid(c.get_mut()) | ||
| #[inline] |
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.
| // SAFETY: This macro ensures that `$repr` and `Self` have the same | ||
| // size and bit validity. Thus, a bit-valid instance of `$repr` is | ||
| // also a bit-valid instance of `Self`. | ||
| #[inline] |
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.
| field_candidate.transmute_unchecked::< | ||
| #zerocopy_crate::wrappers::ReadOnly<#field_tys>, | ||
| _, | ||
| #zerocopy_crate::pointer::cast::CastUnsized | ||
| >() |
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.
There's an extra space at the beginning of this line, causing inconsistent indentation within the unsafe block. Please remove it for better code formatting.
| field_candidate.transmute_unchecked::< | |
| #zerocopy_crate::wrappers::ReadOnly<#field_tys>, | |
| _, | |
| #zerocopy_crate::pointer::cast::CastUnsized | |
| >() | |
| field_candidate.transmute_unchecked::< | |
| #zerocopy_crate::wrappers::ReadOnly<#field_tys>, | |
| _, | |
| #zerocopy_crate::pointer::cast::CastUnsized | |
| >() |
6fe302c to
fdabe13
Compare
7a5cf19 to
f7db04e
Compare
bd5543a to
881fc7d
Compare
f7db04e to
622633b
Compare
881fc7d to
ed830e3
Compare
622633b to
6ab7850
Compare
ed830e3 to
e5db6e6
Compare
6ab7850 to
f08dbdd
Compare
Latest Update: v7 — Compare vs v6
📚 Full Patch History
Links show the diff between the row version and the column version.