-
Notifications
You must be signed in to change notification settings - Fork 139
[WIP] Use ReadOnly in is_bit_valid #2873
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: Gbe8d7edd150d80731c79815685c596ed88460ae7
Are you sure you want to change the base?
[WIP] Use ReadOnly in is_bit_valid #2873
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 to consistently use 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
|
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 ReadOnly wrapper to the Maybe type alias, which is used in is_bit_valid checks. This is a significant safety improvement, as it enforces at the type level that these checks do not perform mutations. The changes correctly propagate this new approach through various macros and implementations, and the refactoring of some macros for better clarity is appreciated. My review focuses on cleaning up some leftover commented-out code from this refactoring effort.
src/impls.rs
Outdated
| // // The only way to implement this function is using an exclusive-aliased | ||
| // // pointer. `UnsafeCell`s cannot be read via shared-aliased pointers | ||
| // // (other than by using `unsafe` code, which we can't use since we can't | ||
| // // guarantee how our users are accessing or modifying the `UnsafeCell`). | ||
| // // | ||
| // // `is_bit_valid` is documented as panicking or failing to monomorphize | ||
| // // if called with a shared-aliased pointer on a type containing an | ||
| // // `UnsafeCell`. In practice, it will always be a monomorphization error. | ||
| // // Since `is_bit_valid` is `#[doc(hidden)]` and only called directly | ||
| // // from this crate, we only need to worry about our own code incorrectly | ||
| // // calling `UnsafeCell::is_bit_valid`. The post-monomorphization error | ||
| // // makes it easier to test that this is truly the case, and also means | ||
| // // that if we make a mistake, it will cause downstream code to fail to | ||
| // // compile, which will immediately surface the mistake and give us a | ||
| // // chance to fix it quickly. | ||
| // let c = candidate.into_exclusive_or_pme(); | ||
|
|
||
| // // SAFETY: Since `UnsafeCell<T>` and `T` have the same layout and bit | ||
| // // validity, `UnsafeCell<T>` is bit-valid exactly when its wrapped `T` | ||
| // // is. Thus, this is a sound implementation of | ||
| // // `UnsafeCell::is_bit_valid`. | ||
| // T::is_bit_valid(c.get_mut()) |
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.
src/pointer/ptr.rs
Outdated
| // /// `Ptr<'a, T, (_, _, _)>` → `Ptr<'a, ReadOnly<T>, (_, _, _)>` | ||
| // impl<'a, T, I> Ptr<'a, T, I> | ||
| // where | ||
| // T: ?Sized, | ||
| // I: Invariants, | ||
| // { | ||
| // /// TODO | ||
| // pub(crate) fn into_read_only<R>( | ||
| // self, | ||
| // ) -> Ptr<'a, crate::ReadOnly<T>, (I::Aliasing, I::Alignment, I::Validity)> | ||
| // where | ||
| // T: Read<I::Aliasing, R>, | ||
| // { | ||
| // let ro = self.transmute::<_, _, (_, _)>(); | ||
| // unsafe { ro.assume_alignment() } | ||
| // } | ||
| // } |
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.
src/util/macro_util.rs
Outdated
| let ptr: Ptr<'_, Dst, _> = ptr.cast::<_, crate::pointer::cast::CastSized, _>(); | ||
|
|
||
| if Dst::is_bit_valid(ptr.forget_aligned()) { | ||
| if Dst::is_bit_valid(ptr.transmute::<_, _, (_, (_, BecauseExclusive))>().forget_aligned()) { |
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 call to .forget_aligned() here seems redundant. The transmute() method on Ptr already returns a Ptr with Unaligned alignment, so forget_aligned() is a no-op in this context. Removing it would make the code slightly cleaner.
| if Dst::is_bit_valid(ptr.transmute::<_, _, (_, (_, BecauseExclusive))>().forget_aligned()) { | |
| if Dst::is_bit_valid(ptr.transmute::<_, _, (_, (_, BecauseExclusive))>()) { |
src/util/macros.rs
Outdated
| fn is_bit_valid<A: crate::pointer::invariant::Reference>(candidate: Maybe<'_, Self, A>) -> bool { | ||
| let c: Maybe<'_, Self, crate::pointer::invariant::Exclusive> = candidate.into_exclusive_or_pme(); | ||
| let c: Maybe<'_, $repr, _> = c.transmute::<_, _, (_, (_, BecauseExclusive))>(); | ||
| // let c: Maybe<'_, Self, crate::pointer::invariant::Exclusive> = candidate.into_exclusive_or_pme(); |
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.
src/wrappers.rs
Outdated
| // enum BecauseReadOnly {} | ||
|
|
||
| // /// Denotes that `src: Ptr<Src, (A, _, SV)>` and `dst: Ptr<Self, (A, _, DV)>`, | ||
| // /// referencing the same referent at the same time, cannot be used by safe code | ||
| // /// to break library safety invariants of `Src` or `Self`. | ||
| // /// | ||
| // /// # Safety | ||
| // /// | ||
| // /// At least one of the following must hold: | ||
| // /// - `Src: Read<A, _>` and `Self: Read<A, _>` | ||
| // /// - `Self: InvariantsEq<Src>`, and, for some `V`: | ||
| // /// - `Dst: TransmuteFrom<Src, V, V>` | ||
| // /// - `Src: TransmuteFrom<Dst, V, V>` | ||
|
|
||
| // // SAFETY: TODO | ||
| // unsafe impl<T: ?Sized, A: Aliasing, V> MutationCompatible<T, A, V, V, BecauseReadOnly> | ||
| // for ReadOnly<T> | ||
| // { | ||
| // } |
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.
81ada6e to
a353963
Compare
ad978ce to
ee75641
Compare
09a6d35 to
6ac888a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## Gbe8d7edd150d80731c79815685c596ed88460ae7 #2873 +/- ##
============================================================================
Coverage ? 91.51%
============================================================================
Files ? 20
Lines ? 5919
Branches ? 0
============================================================================
Hits ? 5417
Misses ? 502
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ee75641 to
6539cee
Compare
6ac888a to
1b4834d
Compare
6539cee to
e2f1bc3
Compare
c83b71f to
db5ec29
Compare
e2f1bc3 to
45ff87c
Compare
db5ec29 to
845c5e7
Compare
45ff87c to
d1f62db
Compare
b6642a7 to
d0f57ea
Compare
d1f62db to
65f15b8
Compare
65f15b8 to
318b671
Compare
d0f57ea to
6cbe1ca
Compare
318b671 to
9230a53
Compare
6cbe1ca to
5cf70a2
Compare
5cf70a2 to
e6fab2d
Compare
gherrit-pr-id: G7691845b6b02e9f3d9578435d732bacfa6ca674f
e6fab2d to
6a4dcd8
Compare
9230a53 to
3c09949
Compare
Latest Update: v19 — Compare vs v18
📚 Full Patch History
Links show the diff between the row version and the column version.