-
Notifications
You must be signed in to change notification settings - Fork 50
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
Possible approach to fix #71 #72
base: master
Are you sure you want to change the base?
Conversation
Only a draft since this does not explain the lifetime parameter in the documentation and since it isn’t clear whether an alternative solution with pub struct OwningRef_<'t, O, T: ?Sized> {
owner: O,
reference: *const T,
marker: PhantomData<&'t T>,
}
type OwningRef<O, T: ?Sized> = OwningRef_<'static, O, T>; would be better (e.g. to reduce breakage). This would, effectlively, be like restricting And it’s not clear what the best choice for more vs. less lifetime parameters for all the type synonyms is. This PR so far took the approach of avoiding implicit |
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.
Regarding the no-implicit 'static
I think I prefer this current approach as well.
reference: *const T, | ||
marker: PhantomData<&'t 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.
The standard library takes the approach to store this as a single reference &'t T
in Ref
:
https://github.com/rust-lang/rust/blob/5a8a44196b3cf099f8c9b0156bd902eaec0b4e5f/library/core/src/cell.rs#L1286-L1289
Although the lifetime is still not correct by itself and we'd still have to downgrade it like we do when it's stored in PhantomData
, it looks like it would not break anything and be closer to what it should be (reference
can't be null, the compiler knows it if specified this way and can perform optimization based on that knowledge).
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.
Actually that seems to have been an error by the std : https://github.com/rust-lang/rust/blob/ff8c8dfbe66701531e3e5e335c28c544d0fbc945/library/core/src/cell.rs#L1332-L1335
reference: *mut T, | ||
marker: PhantomData<&'t 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.
The standard library takes the approach to store this as a single reference &'t mut T
in RefMut
:
https://github.com/rust-lang/rust/blob/5a8a44196b3cf099f8c9b0156bd902eaec0b4e5f/library/core/src/cell.rs#L1662-L1665
Although the lifetime is still not correct by itself and we'd still have to downgrade it like we do when it's stored in PhantomData
, it looks like it would not break anything and be closer to what it should be (reference
can't be null, the compiler knows it if specified this way and can perform optimization based on that knowledge).
Resolves #71.