Skip to content
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

Add Ref/RefMut map_split method #51466

Merged
merged 1 commit into from
Jun 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 126 additions & 19 deletions src/libcore/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -570,11 +570,20 @@ impl Display for BorrowMutError {
}
}

// Values [1, MAX-1] represent the number of `Ref` active
// (will not outgrow its range since `usize` is the size of the address space)
// Values [1, MIN_WRITING-1] represent the number of `Ref` active. Values in
// [MIN_WRITING, MAX-1] represent the number of `RefMut` active. Multiple
// `RefMut`s can only be active at a time if they refer to distinct,
// nonoverlapping components of a `RefCell` (e.g., different ranges of a slice).
//
// `Ref` and `RefMut` are both two words in size, and so there will likely never
// be enough `Ref`s or `RefMut`s in existence to overflow half of the `usize`
// range. Thus, a `BorrowFlag` will probably never overflow. However, this is
// not a guarantee, as a pathological program could repeatedly create and then
// mem::forget `Ref`s or `RefMut`s. Thus, all code must explicitly check for
// overflow in order to avoid unsafety.
type BorrowFlag = usize;
const UNUSED: BorrowFlag = 0;
const WRITING: BorrowFlag = !0;
const MIN_WRITING: BorrowFlag = (!0)/2 + 1; // 0b1000...

impl<T> RefCell<T> {
/// Creates a new `RefCell` containing `value`.
Expand Down Expand Up @@ -775,8 +784,9 @@ impl<T: ?Sized> RefCell<T> {

/// Mutably borrows the wrapped value.
///
/// The borrow lasts until the returned `RefMut` exits scope. The value
/// cannot be borrowed while this borrow is active.
/// The borrow lasts until the returned `RefMut` or all `RefMut`s derived
/// from it exit scope. The value cannot be borrowed while this borrow is
/// active.
///
/// # Panics
///
Expand Down Expand Up @@ -818,8 +828,9 @@ impl<T: ?Sized> RefCell<T> {

/// Mutably borrows the wrapped value, returning an error if the value is currently borrowed.
///
/// The borrow lasts until the returned `RefMut` exits scope. The value cannot be borrowed
/// while this borrow is active.
/// The borrow lasts until the returned `RefMut` or all `RefMut`s derived
/// from it exit scope. The value cannot be borrowed while this borrow is
/// active.
///
/// This is the non-panicking variant of [`borrow_mut`](#method.borrow_mut).
///
Expand Down Expand Up @@ -1010,12 +1021,15 @@ struct BorrowRef<'b> {
impl<'b> BorrowRef<'b> {
#[inline]
fn new(borrow: &'b Cell<BorrowFlag>) -> Option<BorrowRef<'b>> {
match borrow.get() {
WRITING => None,
b => {
borrow.set(b + 1);
Some(BorrowRef { borrow: borrow })
},
let b = borrow.get();
if b >= MIN_WRITING {
None
} else {
// Prevent the borrow counter from overflowing into
// a writing borrow.
assert!(b < MIN_WRITING - 1);
borrow.set(b + 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be slightly more performant if we had just "if b >= MIN_WRITING - 1 { None }" here and skip the assert? I e, return None in case we have run out of Refs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feeling is that that's not the meaning of None. If we wanted to do that, we'd have to also update the documentation, and a) that'd technically be a breaking change (not one that would break any realistic code, but still) and, b) it'd make for a more complicated API. This will almost never happen, so I feel like it's better to hide the details of it from users.

Some(BorrowRef { borrow })
}
}
}
Expand All @@ -1024,7 +1038,7 @@ impl<'b> Drop for BorrowRef<'b> {
#[inline]
fn drop(&mut self) {
let borrow = self.borrow.get();
debug_assert!(borrow != WRITING && borrow != UNUSED);
debug_assert!(borrow < MIN_WRITING && borrow != UNUSED);
self.borrow.set(borrow - 1);
}
}
Expand All @@ -1036,8 +1050,9 @@ impl<'b> Clone for BorrowRef<'b> {
// is not set to WRITING.
let borrow = self.borrow.get();
debug_assert!(borrow != UNUSED);
// Prevent the borrow counter from overflowing.
assert!(borrow != WRITING);
// Prevent the borrow counter from overflowing into
// a writing borrow.
assert!(borrow < MIN_WRITING - 1);
self.borrow.set(borrow + 1);
BorrowRef { borrow: self.borrow }
}
Expand Down Expand Up @@ -1109,6 +1124,37 @@ impl<'b, T: ?Sized> Ref<'b, T> {
borrow: orig.borrow,
}
}

/// Split a `Ref` into multiple `Ref`s for different components of the
/// borrowed data.
///
/// The `RefCell` is already immutably borrowed, so this cannot fail.
///
/// This is an associated function that needs to be used as
/// `Ref::map_split(...)`. A method would interfere with methods of the same
/// name on the contents of a `RefCell` used through `Deref`.
///
/// # Examples
///
/// ```
/// #![feature(refcell_map_split)]
/// use std::cell::{Ref, RefCell};
///
/// let cell = RefCell::new([1, 2, 3, 4]);
/// let borrow = cell.borrow();
/// let (begin, end) = Ref::map_split(borrow, |slice| slice.split_at(2));
/// assert_eq!(*begin, [1, 2]);
/// assert_eq!(*end, [3, 4]);
/// ```
#[unstable(feature = "refcell_map_split", issue = "51476")]
#[inline]
pub fn map_split<U: ?Sized, V: ?Sized, F>(orig: Ref<'b, T>, f: F) -> (Ref<'b, U>, Ref<'b, V>)
where F: FnOnce(&T) -> (&U, &V)
{
let (a, b) = f(orig.value);
let borrow = orig.borrow.clone();
(Ref { value: a, borrow }, Ref { value: b, borrow: orig.borrow })
}
}

#[unstable(feature = "coerce_unsized", issue = "27732")]
Expand Down Expand Up @@ -1157,6 +1203,44 @@ impl<'b, T: ?Sized> RefMut<'b, T> {
borrow: borrow,
}
}

/// Split a `RefMut` into multiple `RefMut`s for different components of the
/// borrowed data.
///
/// The underlying `RefCell` will remain mutably borrowed until both
/// returned `RefMut`s go out of scope.
///
/// The `RefCell` is already mutably borrowed, so this cannot fail.
///
/// This is an associated function that needs to be used as
/// `RefMut::map_split(...)`. A method would interfere with methods of the
/// same name on the contents of a `RefCell` used through `Deref`.
///
/// # Examples
///
/// ```
/// #![feature(refcell_map_split)]
/// use std::cell::{RefCell, RefMut};
///
/// let cell = RefCell::new([1, 2, 3, 4]);
/// let borrow = cell.borrow_mut();
/// let (mut begin, mut end) = RefMut::map_split(borrow, |slice| slice.split_at_mut(2));
/// assert_eq!(*begin, [1, 2]);
/// assert_eq!(*end, [3, 4]);
/// begin.copy_from_slice(&[4, 3]);
/// end.copy_from_slice(&[2, 1]);
/// ```
#[unstable(feature = "refcell_map_split", issue = "51476")]
#[inline]
pub fn map_split<U: ?Sized, V: ?Sized, F>(
orig: RefMut<'b, T>, f: F
) -> (RefMut<'b, U>, RefMut<'b, V>)
where F: FnOnce(&mut T) -> (&mut U, &mut V)
{
let (a, b) = f(orig.value);
let borrow = orig.borrow.clone();
(RefMut { value: a, borrow }, RefMut { value: b, borrow: orig.borrow })
}
}

struct BorrowRefMut<'b> {
Expand All @@ -1167,22 +1251,45 @@ impl<'b> Drop for BorrowRefMut<'b> {
#[inline]
fn drop(&mut self) {
let borrow = self.borrow.get();
debug_assert!(borrow == WRITING);
self.borrow.set(UNUSED);
debug_assert!(borrow >= MIN_WRITING);
self.borrow.set(if borrow == MIN_WRITING {
UNUSED
} else {
borrow - 1
});
}
}

impl<'b> BorrowRefMut<'b> {
#[inline]
fn new(borrow: &'b Cell<BorrowFlag>) -> Option<BorrowRefMut<'b>> {
// NOTE: Unlike BorrowRefMut::clone, new is called to create the initial
// mutable reference, and so there must currently be no existing
// references. Thus, while clone increments the mutable refcount, here
// we simply go directly from UNUSED to MIN_WRITING.
match borrow.get() {
UNUSED => {
borrow.set(WRITING);
borrow.set(MIN_WRITING);
Some(BorrowRefMut { borrow: borrow })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this method maybe get a comment that it deliberately doesn't increment, because this is what is called by RefCell::borrow_mut() and hence must only succeed if there is no writer at all yet -- unlike BorrowRefMut::clone()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

},
_ => None,
}
}

// Clone a `BorrowRefMut`.
//
// This is only valid if each `BorrowRefMut` is used to track a mutable
// reference to a distinct, nonoverlapping range of the original object.
// This isn't in a Clone impl so that code doesn't call this implicitly.
#[inline]
fn clone(&self) -> BorrowRefMut<'b> {
let borrow = self.borrow.get();
debug_assert!(borrow >= MIN_WRITING);
// Prevent the borrow counter from overflowing.
assert!(borrow != !0);
self.borrow.set(borrow + 1);
BorrowRefMut { borrow: self.borrow }
}
}

/// A wrapper type for a mutably borrowed value from a `RefCell<T>`.
Expand Down
58 changes: 58 additions & 0 deletions src/libcore/tests/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,64 @@ fn ref_map_does_not_update_flag() {
assert!(x.try_borrow_mut().is_ok());
}

#[test]
fn ref_map_split_updates_flag() {
let x = RefCell::new([1, 2]);
{
let b1 = x.borrow();
assert!(x.try_borrow().is_ok());
assert!(x.try_borrow_mut().is_err());
{
let (_b2, _b3) = Ref::map_split(b1, |slc| slc.split_at(1));
assert!(x.try_borrow().is_ok());
assert!(x.try_borrow_mut().is_err());
}
assert!(x.try_borrow().is_ok());
assert!(x.try_borrow_mut().is_ok());
}
assert!(x.try_borrow().is_ok());
assert!(x.try_borrow_mut().is_ok());

{
let b1 = x.borrow_mut();
assert!(x.try_borrow().is_err());
assert!(x.try_borrow_mut().is_err());
{
let (_b2, _b3) = RefMut::map_split(b1, |slc| slc.split_at_mut(1));
assert!(x.try_borrow().is_err());
assert!(x.try_borrow_mut().is_err());
drop(_b2);
assert!(x.try_borrow().is_err());
assert!(x.try_borrow_mut().is_err());
}
assert!(x.try_borrow().is_ok());
assert!(x.try_borrow_mut().is_ok());
}
assert!(x.try_borrow().is_ok());
assert!(x.try_borrow_mut().is_ok());
}

#[test]
fn ref_map_split() {
let x = RefCell::new([1, 2]);
let (b1, b2) = Ref::map_split(x.borrow(), |slc| slc.split_at(1));
assert_eq!(*b1, [1]);
assert_eq!(*b2, [2]);
}

#[test]
fn ref_mut_map_split() {
let x = RefCell::new([1, 2]);
{
let (mut b1, mut b2) = RefMut::map_split(x.borrow_mut(), |slc| slc.split_at_mut(1));
assert_eq!(*b1, [1]);
assert_eq!(*b2, [2]);
b1[0] = 2;
b2[0] = 1;
}
assert_eq!(*x.borrow(), [2, 1]);
}

#[test]
fn ref_map_accessor() {
struct X(RefCell<(u32, char)>);
Expand Down
1 change: 1 addition & 0 deletions src/libcore/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#![feature(pattern)]
#![feature(range_is_empty)]
#![feature(raw)]
#![feature(refcell_map_split)]
#![feature(refcell_replace_swap)]
#![feature(slice_patterns)]
#![feature(slice_rotate)]
Expand Down