-
Notifications
You must be signed in to change notification settings - Fork 105
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
[project] Initial commit #292
base: main
Are you sure you want to change the base?
Conversation
This is basically ready, except there's one outstanding safety doc comment that I can't figure out how to write. I know the safety reasoning here is pretty convoluted, so I'd love folks to double-check my work and/or suggest ways to simplify or strengthen the arguments. Referring to #196 may help in understanding the design. |
src/project.rs
Outdated
/// `Self` must be a `repr(transparent)`, `repr(packed)`, or `repr(C)` struct | ||
/// with a single non-zero-sized field of type `Self::Inner`. `W` must be a | ||
/// `repr(transparent)`, `repr(packed)`, or `repr(C)` struct with a single | ||
/// non-zero field of type `F`. |
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.
Oops, this is just wrong. E.g., it doesn't apply to MaybeValid<T: AsMaybeUninit>
from #279, whose inner field is not T
, but T::MaybeUninit
.
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.
Okay, I think this should be fixed now?
// TODO(#196): Is there any way to make sure this calls | ||
// `Borrow::borrow` or `BorrowMut::borrow_mut` even if the type has | ||
// an inherent method of the same name? |
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 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.
@kupiakos I wasn't actually able to get this to work on this PR. Did you test with this PR, or just a minimized version?
35847cb
to
1bd5834
Compare
Makes progress on #196
1bd5834
to
ca3cf29
Compare
Makes progress on #196