-
Notifications
You must be signed in to change notification settings - Fork 137
[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: G17f9a0ab1a99ba2147dbc334934cb9e1f3709128
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 @@
## G17f9a0ab1a99ba2147dbc334934cb9e1f3709128 #2873 +/- ##
=============================================================================
- Coverage 91.55% 91.51% -0.04%
=============================================================================
Files 20 20
Lines 5906 5916 +10
=============================================================================
+ Hits 5407 5414 +7
- Misses 499 502 +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
5718b7a to
3c44f90
Compare
91e2600 to
054d5d4
Compare
3c44f90 to
f6911ab
Compare
af9e006 to
c511655
Compare
20452c3 to
fdf2d74
Compare
c511655 to
ecb8b50
Compare
fdf2d74 to
fc38019
Compare
0c1b442 to
f02e8df
Compare
c2d0307 to
acc181c
Compare
187a61f to
de2e502
Compare
d48bb99 to
2e0baa1
Compare
de2e502 to
9f2289d
Compare
2e0baa1 to
1262012
Compare
9f2289d to
129800e
Compare
bd56d16 to
f743f9b
Compare
129800e to
6c1c181
Compare
f743f9b to
59c2a36
Compare
6c1c181 to
878bd6b
Compare
Makes progress on #2336 gherrit-pr-id: G7691845b6b02e9f3d9578435d732bacfa6ca674f
878bd6b to
532249e
Compare
59c2a36 to
1569756
Compare
Makes progress on #2336
TryFromByteson non-Immutableunions #2876ReadOnly<T>which is unconditionallyImmutable#2866Latest Update: v54 — Compare vs v53
📚 Full Patch History
Links show the diff between the row version and the column version.