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

Extend Cell to work with non-Copy types #39287

Merged
merged 4 commits into from
Feb 3, 2017
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
12 changes: 7 additions & 5 deletions src/doc/book/choosing-your-guarantees.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ These types are _generally_ found in struct fields, but they may be found elsewh

## `Cell<T>`

[`Cell<T>`][cell] is a type that provides zero-cost interior mutability, but only for `Copy` types.
[`Cell<T>`][cell] is a type that provides zero-cost interior mutability by moving data in and
out of the cell.
Since the compiler knows that all the data owned by the contained value is on the stack, there's
no worry of leaking any data behind references (or worse!) by simply replacing the data.

Expand Down Expand Up @@ -160,24 +161,25 @@ This relaxes the &ldquo;no aliasing with mutability&rdquo; restriction in places
unnecessary. However, this also relaxes the guarantees that the restriction provides; so if your
invariants depend on data stored within `Cell`, you should be careful.

This is useful for mutating primitives and other `Copy` types when there is no easy way of
This is useful for mutating primitives and other types when there is no easy way of
doing it in line with the static rules of `&` and `&mut`.

`Cell` does not let you obtain interior references to the data, which makes it safe to freely
mutate.

#### Cost

There is no runtime cost to using `Cell<T>`, however if you are using it to wrap larger (`Copy`)
There is no runtime cost to using `Cell<T>`, however if you are using it to wrap larger
structs, it might be worthwhile to instead wrap individual fields in `Cell<T>` since each write is
otherwise a full copy of the struct.


## `RefCell<T>`

[`RefCell<T>`][refcell] also provides interior mutability, but isn't restricted to `Copy` types.
[`RefCell<T>`][refcell] also provides interior mutability, but doesn't move data in and out of the
cell.

Instead, it has a runtime cost. `RefCell<T>` enforces the read-write lock pattern at runtime (it's
However, it has a runtime cost. `RefCell<T>` enforces the read-write lock pattern at runtime (it's
like a single-threaded mutex), unlike `&T`/`&mut T` which do so at compile time. This is done by the
`borrow()` and `borrow_mut()` functions, which modify an internal reference count and return smart
pointers which can be dereferenced immutably and mutably respectively. The refcount is restored when
Expand Down
149 changes: 108 additions & 41 deletions src/libcore/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,18 @@
//! references. We say that `Cell<T>` and `RefCell<T>` provide 'interior mutability', in contrast
//! with typical Rust types that exhibit 'inherited mutability'.
//!
//! Cell types come in two flavors: `Cell<T>` and `RefCell<T>`. `Cell<T>` provides `get` and `set`
//! methods that change the interior value with a single method call. `Cell<T>` though is only
//! compatible with types that implement `Copy`. For other types, one must use the `RefCell<T>`
//! type, acquiring a write lock before mutating.
//! Cell types come in two flavors: `Cell<T>` and `RefCell<T>`. `Cell<T>` implements interior
//! mutability by moving values in and out of the `Cell<T>`. To use references instead of values,
//! one must use the `RefCell<T>` type, acquiring a write lock before mutating. `Cell<T>` provides
//! methods to retrieve and change the current interior value:
//!
//! - For types that implement `Copy`, the `get` method retrieves the current interior value.
//! - For types that implement `Default`, the `take` method replaces the current interior value
//! with `Default::default()` and returns the replaced value.
//! - For all types, the `replace` method replaces the current interior value and returns the
//! replaced value and the `into_inner` method consumes the `Cell<T>` and returns the interior
//! value. Additionally, the `set` method replaces the interior value, dropping the replaced
//! value.
//!
//! `RefCell<T>` uses Rust's lifetimes to implement 'dynamic borrowing', a process whereby one can
//! claim temporary, exclusive, mutable access to the inner value. Borrows for `RefCell<T>`s are
Expand Down Expand Up @@ -176,9 +184,10 @@
use cmp::Ordering;
use fmt::{self, Debug, Display};
use marker::Unsize;
use mem;
use ops::{Deref, DerefMut, CoerceUnsized};

/// A mutable memory location that admits only `Copy` data.
/// A mutable memory location.
///
/// See the [module-level documentation](index.html) for more.
#[stable(feature = "rust1", since = "1.0.0")]
Expand All @@ -187,23 +196,6 @@ pub struct Cell<T> {
}

impl<T:Copy> Cell<T> {
/// Creates a new `Cell` containing the given value.
///
/// # Examples
///
/// ```
/// use std::cell::Cell;
///
/// let c = Cell::new(5);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub const fn new(value: T) -> Cell<T> {
Cell {
value: UnsafeCell::new(value),
}
}

/// Returns a copy of the contained value.
///
/// # Examples
Expand All @@ -221,25 +213,6 @@ impl<T:Copy> Cell<T> {
unsafe{ *self.value.get() }
}

/// Sets the contained value.
///
/// # Examples
///
/// ```
/// use std::cell::Cell;
///
/// let c = Cell::new(5);
///
/// c.set(10);
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn set(&self, value: T) {
unsafe {
*self.value.get() = value;
}
}

/// Returns a reference to the underlying `UnsafeCell`.
///
/// # Examples
Expand Down Expand Up @@ -378,6 +351,100 @@ impl<T: Copy> From<T> for Cell<T> {
}
}

impl<T> Cell<T> {
/// Creates a new `Cell` containing the given value.
///
/// # Examples
///
/// ```
/// use std::cell::Cell;
///
/// let c = Cell::new(5);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
pub const fn new(value: T) -> Cell<T> {
Copy link
Member

Choose a reason for hiding this comment

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

I think doing stable inside unstable makes the item stable again. At least that was the problem of bug #38860

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure what the right thing to do here is. There needs to be some way to construct a non-Copy Cell but perhaps it would be better to add a new function?

Copy link
Member

Choose a reason for hiding this comment

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

I would advise moving the unstable attribute downward and applying it to each of the new methods. That generally makes it easier to track stabilization later, anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do that. Do you think I should create a new function instead of moving new() here or is this ok?

Copy link
Member

Choose a reason for hiding this comment

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

I think new should be modified, given that we're changing set as well. But cc @rust-lang/libs (since this makes the change roughly insta-stable).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I considered this as an implication of accepting the RFC, so immediately generalizing the behavior of new and set seems ok to me.

Cell {
value: UnsafeCell::new(value),
}
}

/// Sets the contained value.
///
/// # Examples
///
/// ```
/// use std::cell::Cell;
///
/// let c = Cell::new(5);
///
/// c.set(10);
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn set(&self, val: T) {
let old = self.replace(val);
drop(old);
Comment on lines +386 to +387
Copy link
Contributor

Choose a reason for hiding this comment

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

@wesleywiser do we actually need old here? Also why the explicit drop? Mostly just trying to make sure I'm not missing some subtle detail :)

Copy link
Member

Choose a reason for hiding this comment

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

This is a comment on a very old PR. :D But no, that drop is not necessary, it probably was just there for explicitness.

}

/// Replaces the contained value.
///
/// # Examples
///
/// ```
/// #![feature(move_cell)]
/// use std::cell::Cell;
///
/// let c = Cell::new(5);
/// let old = c.replace(10);
///
/// assert_eq!(5, old);
/// ```
#[unstable(feature = "move_cell", issue = "39264")]
pub fn replace(&self, val: T) -> T {
mem::replace(unsafe { &mut *self.value.get() }, val)
}

/// Unwraps the value.
///
/// # Examples
///
/// ```
/// #![feature(move_cell)]
/// use std::cell::Cell;
///
/// let c = Cell::new(5);
/// let five = c.into_inner();
///
/// assert_eq!(five, 5);
/// ```
#[unstable(feature = "move_cell", issue = "39264")]
pub fn into_inner(self) -> T {
unsafe { self.value.into_inner() }
}
}

impl<T: Default> Cell<T> {
/// Takes the value of the cell, leaving `Default::default()` in its place.
///
/// # Examples
///
/// ```
/// #![feature(move_cell)]
/// use std::cell::Cell;
///
/// let c = Cell::new(5);
/// let five = c.take();
///
/// assert_eq!(five, 5);
/// assert_eq!(c.into_inner(), 0);
/// ```
#[unstable(feature = "move_cell", issue = "39264")]
pub fn take(&self) -> T {
self.replace(Default::default())
}
}

#[unstable(feature = "coerce_unsized", issue = "27732")]
impl<T: CoerceUnsized<U>, U> CoerceUnsized<Cell<U>> for Cell<T> {}

Expand Down
31 changes: 31 additions & 0 deletions src/libcoretest/cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,37 @@ fn cell_default() {
assert_eq!(0, cell.get());
}

#[test]
fn cell_set() {
let cell = Cell::new(10);
cell.set(20);
assert_eq!(20, cell.get());

let cell = Cell::new("Hello".to_owned());
cell.set("World".to_owned());
assert_eq!("World".to_owned(), cell.into_inner());
}

#[test]
fn cell_replace() {
let cell = Cell::new(10);
assert_eq!(10, cell.replace(20));
assert_eq!(20, cell.get());

let cell = Cell::new("Hello".to_owned());
assert_eq!("Hello".to_owned(), cell.replace("World".to_owned()));
assert_eq!("World".to_owned(), cell.into_inner());
}

#[test]
fn cell_into_inner() {
let cell = Cell::new(10);
assert_eq!(10, cell.into_inner());

let cell = Cell::new("Hello world".to_owned());
assert_eq!("Hello world".to_owned(), cell.into_inner());
}

#[test]
fn refcell_default() {
let cell: RefCell<u64> = Default::default();
Expand Down
1 change: 1 addition & 0 deletions src/libcoretest/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#![feature(ordering_chaining)]
#![feature(result_unwrap_or_default)]
#![feature(ptr_unaligned)]
#![feature(move_cell)]

extern crate core;
extern crate test;
Expand Down