various improvements for the new design

[Freax13]

This pr brings various improvements for the new design:

• all methods take Self by value, not by reference
  • see New design based on *mut T #22 (comment)
• implement Copy for VolatilePtr
  • This is a requirement for the previous change. VolatilePtr<'a, T> now behaves a lot like &'a Cell<T> in that it allows reading and writing to a memory location with a copy-able non-unique handle. Replaced by borrow and borrow_mut.
  • See https://github.com/Freax13/volatile/tree/copy-again for a branch that still has this change
• add VolatilePtr::as_slice_mut
  • VolatilePtr::as_slice already existed, but didn't give out writable VolatilePtrs
• add VolatilePtr::iter and VolatilePtr::iter_mut to make it easier to iterator over all elements of a slice
• add VolatilePtr::<[T]>::is_empty
  • we already had VolatilePtr::<[T]>::len, so users would probably expect .is_empty() to work too
• add some missing where bounds to methods that read or write memory
• make map_field! reject unaligned fields
• add #[inline] to most functions Generic types are inlineable anyways

I didn't want to create multiple prs, so this pr has unrelated changes. Feel free to pick and choose which improvements you want to use in the new design.

I've been using the new design pretty much since #22 was created. I worked around some of the missing functions (e.g. as_slice_mut), but I couldn't do that for some of the bigger changes such as making methods take Self by value. So, instead of adding a new layer of workarounds, I decided to ignore that the #22 is not done yet and create a pr to it.

Related to #22

[phil-opp]

Thanks a lot for opening this and for continuing the work on the new design!

Most of the improvements look good to me, but I don't think that implementing Clone/Copy and taking self by value everywhere is a good idea. With that change, all write operations would need to be unsafe again because a copy or clone can add mutable aliasing at any time. We also would need to make the VolatilePtr type !Sync because concurrent writes would be undefined behavior. So we would not gain any additional safety guarantees over *mut T anymore.

To solve the problem that you mentioned in #22 (comment), we could add additional *_into methods. So we would have split_at(&self,...), split_at_mut(&mut self, ...), and split_at_into(self, ...). Depending on your use case, they could all be useful. This approach is also used by other crates such as ndarray: For example, it's ArrayView type has an into_slice method in addition to its as_slice_mut and as_slice methods.

[Freax13]

Most of the improvements look good to me, but I don't think that implementing Clone/Copy and taking self by value everywhere is a good idea. With that change, all write operations would need to be unsafe again because a copy or clone can add mutable aliasing at any time. We also would need to make the VolatilePtr type !Sync because concurrent writes would be undefined behavior. So we would not gain any additional safety guarantees over *mut T anymore.

I wouldn't say that we don't gain any safety guarantess over *mut T, we still know that the pointer points to valid live data. But yeah, we'd need all types to be !Send and !Sync which is a significant downside.

To solve the problem that you mentioned in #22 (comment), we could add additional *_into methods. So we would have split_at(&self,...), split_at_mut(&mut self, ...), and split_at_into(self, ...). Depending on your use case, they could all be useful. This approach is also used by other crates such as ndarray: For example, it's ArrayView type has an into_slice method in addition to its as_slice_mut and as_slice methods.

The problem with that is that, we'd need to duplicate a lot of different methods:

• map
• map_const
• map_mut
• map_mut_const
• index
• index_const
• index_mut
• index_mut_const
• iter
• iter_mut
• split_at
• split_at_mut
• split_at_unchecked
• split_at_mut_unchecked
• as_chunks
• as_chunks_unchecked
• as_chunks_mut
• as_chunks_unchecked_mut
• as_slice
• as_slice_mut

And maybe read_only and write_only.

Another way that doesn't need duplicate methods, but is probably very verbose, would be to force explicitly reborrowing by making all those methods I listed above take Self by value and add two new methods VolatilePtr<T>::reborrow(&'a self) -> VolatilePtr<'a. T, Access<R, NoAccess>> and VolatilePtr<T>::reborrow_mut(&'a mut self) -> VolatilePtr<'a. T, Access<R, W>> that could be used to create a new instance with a shorter lifetime.

[phil-opp]

The reborrow/reborrow_mut methods are an interesting idea! A single reborrow call at the beginning of a method chain might be enough, so maybe the verbosity is manageable. (Bikeshedding: I would call the methods just borrow/borrow_mut, I think that's easier to understand.)

If this approach doesn't work for some reason, I'm also fine with duplicating the relevant methods. It's a bit of duplication, but I don't think that this is really a problem. We already have &self and &mut self flavors for most methods, so adding a third self flavor would be fine with me. (Also, your list above is twice as long as it needs to be. We only need one new map_into variant, not both map_into and map_mut_into.)

[Freax13]

I've pushed my current work-in-progress state. Most of the functionally should be implemented, but it's still missing documentation.

I think if we go with this approach we can also get rid of the *_mut methods because all methods now take the owned value instead of a reference anyways, so we don't need to distinguish between shared and mutable references. I'm not quite sure if that would a good thing though.

[Freax13]

A single reborrow call at the beginning of a method chain might be enough, so maybe the verbosity is manageable.

Unfortunatly this doesn't work for map_field[_mut]!, I find myself quite often creating temporary bindings to reborrowed pointers:

Ideally the first block would just be operational_registers_ptr.configure.set_max_device_slots_enabled(max_slots);.

[josephlr]

First of all, I like this design. Having the VolatilePtr be Clone/Copy and have by-value methods better reflects what's actually going on (in my opinion). It also makes code like that linked above by @Freax13 easier to write and less verbose.

The main downside of VolatilePtr not being Sync/Send by default actually seems fine to me. Multi-threaded volatile access is tricky and is potentially implementation and/or target specific. The synchronization rules may also be different for different memory regions. See the discussion between @Freax13 and @Lokathor in #22 (comment)

Given the complexities and subtleties involved here, I think requiring a user to override our default of !Send/!Sync (say via a wrapper type) is not unreasonable. One thing that might make things more ergonomic would be to make VolatilePtr<T> Send/Sync under the following conditions:

• Safe writes are not allowed on the pointer type
  • The Write component of Access is either NoAccess or UnsafeAccess
  • Alternatively, if that is too complicated, we could require that not Write operations are allowed at all. I.E. the Write access must be NoAccess.
• T: Sync

This would match the traits for &T (which a VolatilePtr<T, ReadOnly> roughly acts like):

• &T: Copy (unconditinaolly)
• &T: Sync if and only if T: Sync
• &T: Send if and only if T: Sync

[phil-opp]

@josephlr Thanks a lot for your thoughts! I'm sorry for replying so late.

I fully agree that making Volatile a Copy type makes it easier to use. My main concern with this design is whether we can still make the read/write methods safe without causing undefined behavior. If not, there would not be a big advantage in using Volatile instead of using core::ptr::read_volatile directly.

Given that this PR in its current state is a strict improvement to #22, I think it's a good idea to merge it and continue the discussion in #22.

[phil-opp]

@Freax13 Thanks a lot for your work on this PR and sorry for the long delay!