-
Notifications
You must be signed in to change notification settings - Fork 138
[WIP] Remove MutationCompatible, Read #2878
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: G27dd847426e3572e87b20bd64642c9fcb0e51ec8
Are you sure you want to change the base?
[WIP] Remove MutationCompatible, Read #2878
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 undertakes a significant internal 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
|
847fe34 to
70fb472
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 is a significant and welcome refactoring that removes the MutationCompatible and Read traits, simplifying the trait hierarchy and making type signatures less complex. The changes are extensive, touching many parts of the codebase, but they appear to be consistent and correct. The introduction of new TryTransmuteFromPtr impls to replace the old traits is a good approach. I've noticed you've added several TODO comments to track necessary updates to safety documentation, which is great for a work-in-progress PR. I've raised a few review comments to formalize the need to address these TODOs, particularly for the new unsafe implementations, to ensure they are fully justified before this PR is merged.
| unsafe { Ptr::from_inner(inner) }) | ||
| })?; | ||
|
|
||
| // TODO: Update this safety comment for `TryTransmuteFromPtr`. |
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.
| // TODO: Maybe use the same reason for both? There shouldn't be any situation in | ||
| // which the reasons are different (one is BecauseExclusive and the other is | ||
| // BecauseImmutable). | ||
| // SAFETY: TODO |
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: `Src: Read<A, _>` and `Dst: Read<A, _>`. | ||
| unsafe impl<Src: ?Sized, Dst: ?Sized, A: Aliasing, SV: Validity, DV: Validity, R> | ||
| MutationCompatible<Src, A, SV, DV, (BecauseRead, R)> for Dst | ||
| // SAFETY: TODO |
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## G27dd847426e3572e87b20bd64642c9fcb0e51ec8 #2878 +/- ##
============================================================================
Coverage ? 91.44%
============================================================================
Files ? 20
Lines ? 5912
Branches ? 0
============================================================================
Hits ? 5406
Misses ? 506
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
050ea06 to
2972d07
Compare
70fb472 to
3c218ea
Compare
f5919bc to
9562030
Compare
3c218ea to
741e5f8
Compare
9562030 to
625fa9b
Compare
741e5f8 to
91658ad
Compare
74741ec to
a8fdf22
Compare
256fa34 to
7a4c315
Compare
a8fdf22 to
4314268
Compare
7a4c315 to
88add0a
Compare
4314268 to
10d1c38
Compare
88add0a to
4a75a90
Compare
6cde490 to
152c38f
Compare
4a75a90 to
1f5ac66
Compare
1f5ac66 to
d3c8fb4
Compare
152c38f to
4959305
Compare
d3c8fb4 to
b201ea1
Compare
4959305 to
6f92cae
Compare
b201ea1 to
62478e2
Compare
6f92cae to
3fb9013
Compare
`MutationCompatible` introduces unnecessary indirection and makes many type signatures needlessly complex. In this commit, we flatten the trait hierarchy so that `TryTransmuteFromPtr` has more impls, and `MutationCompatible` is removed entirely. We also remove the `Read` trait for a similar reason. gherrit-pr-id: G8a3e5c48698ed092b1175cfb42f6f85fb3fb6a34
62478e2 to
e6bfe16
Compare
3fb9013 to
b685399
Compare
MutationCompatibleintroduces unnecessary indirection and makes manytype signatures needlessly complex. In this commit, we flatten the trait
hierarchy so that
TryTransmuteFromPtrhas more impls, andMutationCompatibleis removed entirely.We also remove the
Readtrait for a similar reason.Latest Update: v17 — Compare vs v16
📚 Full Patch History
Links show the diff between the row version and the column version.