-
Notifications
You must be signed in to change notification settings - Fork 138
[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.39% 91.44% +0.05%
=============================================================================
Files 20 20
Lines 5912 5916 +4
=============================================================================
+ Hits 5403 5410 +7
+ Misses 509 506 -3 ☔ 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
b8a99e3 to
8eb87e8
Compare
5db40fd to
a916f8f
Compare
8eb87e8 to
aaf6eba
Compare
a916f8f to
ca87cfd
Compare
aaf6eba to
8df726e
Compare
ca87cfd to
56473d2
Compare
2337b41 to
5d01292
Compare
56473d2 to
374d785
Compare
5d01292 to
b71eaa7
Compare
2443118 to
ab5e943
Compare
b71eaa7 to
554d331
Compare
ab5e943 to
5718b7a
Compare
554d331 to
91e2600
Compare
5718b7a to
3c44f90
Compare
91e2600 to
054d5d4
Compare
3c44f90 to
f6911ab
Compare
af9e006 to
c511655
Compare
20452c3 to
fdf2d74
Compare
c511655 to
ecb8b50
Compare
gherrit-pr-id: G7691845b6b02e9f3d9578435d732bacfa6ca674f
fdf2d74 to
fc38019
Compare
ecb8b50 to
0c1b442
Compare
Latest Update: v44 — Compare vs v43
📚 Full Patch History
Links show the diff between the row version and the column version.