-
Notifications
You must be signed in to change notification settings - Fork 19
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
[v0.5] New design with two wrapper types: VolatilePtr
and VolatileRef
#29
Conversation
The lifetime parameter is required to make `map_mut` sound (otherwise we might get mutable aliasing). Also: - add new `from_ref` and `from_mut_ref` contructor methods again. - add a `new_generic` constructor method to construct a `VolatilePtr` with arbitrary lifetime and access parameters.
…on `VolatileCell`
The methods are still unsable through `as_ptr`/`as_mut_ptr`.
- Remove potentially unsafe constructor methods from `&T`/`&mut T`. They're still available on `VolatileRef` if needed. - Refactor: Move most method implementations to new submodule. - Update docs and examples - Rename `as_ptr` method to `as_raw_ptr` to reduce confusion
Update:
I think this is the most promising solution. We need nicer names though, so I renamed the I also drafted up a short introduction of the two types for our crate docs:
Given that the API of the current release is not sound, I'm favor of merging this PR soon. We can continue improving the API and implementation in future PRs. The only thing that I want to do before merging is to write proper docs for all methods. |
VolatilePtrCopy
or VolatilePtrSend
?VolatilePtr
and VolatileRef
I'm trying to page things back into my memory, but this sounds like basically a "more restrictive than theoretically necessary, but at least we know it is sound" solution. Is that right? If so, I would agree to merge it now and if possible improve it later (again, if possible). |
There is a relevant discussion on having a |
There is no reason to have separate methods now that all methods take `self` by value.
Published as v0.5.0 |
This draft PR is a follow-up to #22. There was a lot of good discussion on that PR, mostly about the question whether the volatile wrapper type should implement
Copy
orSend
. Implementing both of these traits would not be safe because it would allow unsynchronized concurrent writes from different threads.I think that both approaches are valid, each with its advantages and drawbacks. For this reason, I tried to implement them both in this PR, as a base for further discussion. The following table summarizes the fundamental differences between the two pointer types:
VolatilePtr
VolatileRef
Copy
, but are notSend
.Send
ifT: Sync
and implementCopy
only if no mutation is allowed.Mutex
typesMutex types require
Send
, butVolatilePtrCopy
is!Send
because writing the pointer from different threads would be UB.Workaround: Create a custom wrapper type that unsafely implements
Send
Type can safely implement
Send
since writes require a&mut
reference and type is not clonable.read
andwrite
methods are safe to call (after unsafe construction)Implements
Copy
, so multiple instances can point to the same address. However, the instances are bound to a single thread, so concurrent writes should not be possible. We never construct&mut
references, so we should not have mutable aliasing.Writes require an
&mut self
and the type is not clonable, so concurrent writes should not be possible even when used concurrently.No borrow conflicts since all methods simply copy the pointer
Methods borrow self, which requires manual reborrows and makes splitting struct pointers into field pointers difficult.
Workaround: We could create an unsafe
duplicate
function that could be used for splitting a struct pointer into multiple field pointers. Maybe it's also possible to create a safe macro for this use case.To simplify this PR and focus the discussion, I removed the unsafe access types for now. We can add them later once we settled on a fundamental API design. I also moved the
unstable
andvery_unstable
functions to separate submodules, so we can ignore them too for now.Fixes #39