Align RwLockWriteGuard fields with the other RwLock guards (+ cleanup) by connortsui20 · Pull Request #140018 · rust-lang/rust (original) (raw)

Tracking Issue: #138559

Relevant comment: #138559 (comment)

Summary

This PR does a few things as setup for the implementation of try_upgrade on RwLock. They are mostly cosmetic changes.

• Unifies the lifetime annotations of all implementation blocks to all use 'rwlock instead of using 'a
• Unifies the implementation blocks for the 4 different kinds of guards (they each had 2 impl blocks before, so I just copied and pasted them into the same one)
• Adds documentation to the fields of each struct
  • Renamed the poison fields to poison_guard (if this change makes sense, we should also rename the poison field in MutexGuard)
• Aligns the fields of RwLockWriteGuard to be similar to the other 3 guards

Changes

The only non-cosmetic change here is the last point, in which the fields of RwLockWriteGuard are different.

/// (For reference) pub struct RwLock<T: ?Sized> { inner: sys::RwLock, poison: poison::Flag, data: UnsafeCell, }

/// Before pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> { lock: &'a RwLock, poison: poison::Guard, }

/// After pub struct RwLockWriteGuard<'rwlock, T: ?Sized + 'rwlock> { /// A pointer to the data protected by the RwLock. NonNull is preferable over *mut T to /// allow for niche optimizations. /// Note that we use a pointer here instead of &'rwlock mut T to avoid noalias violations, /// because a RwLockWriteGuard instance only holds uniqueness until it drops, not for its /// whole scope. data: NonNull, /// NonNull is covariant over T, so we add a PhantomData<&'rwlock mut T> field below for /// the correct variance over T (invariance). _variance: PhantomData<&'rwlock mut T>, /// A reference back to the original (internal) lock. inner_lock: &'rwlock sys::RwLock, /// A reference to the original RwLock's poison state. poison_flag: &'rwlock poison::Flag, /// The poison guard. See the [poison] module for more information. poison_guard: poison::Guard, }

As a TLDR for #138559 (comment), if we want to convert a RwLockReadGuard into a RwLockWriteGuard safely (as we will need to for try_upgrade), I'm pretty sure we need to make the fields the same otherwise we have to do some nasty transmuting. Note it is very possible that I could be wrong on this.

Concerns

I do have a concern over the noalias comment that I essentially copied and pasted from the docs for MappedRwLockWriteGuard. Is it really true that making the data field NonNull here avoids noalias LLVM violations in the exclusive / unique setting? This description makes sense for the read variants of the guards, but I'm having trouble wrapping my head around the logic for the write variants.

I am also somewhat concerned because MutexGuard only stores a reference while MappedMutexGuard stores a pointer.

The only reason I have it here is because MappedRwLockWriteGuard was merged in #117107. CC'ing @zachs18 since he wrote that PR.