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 &ReadOnlyCell<T> conversions from &T and &Cell<T> #3580

Closed
wants to merge 1 commit into from
Closed
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
57 changes: 48 additions & 9 deletions src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -641,20 +641,38 @@ impl<T> Drop for PyBuffer<T> {
/// The data cannot be modified through the reference, but other references may
/// be modifying the data.
#[repr(transparent)]
pub struct ReadOnlyCell<T: Element>(cell::UnsafeCell<T>);

impl<T: Element> ReadOnlyCell<T> {
/// Returns a copy of the current value.
#[inline]
pub fn get(&self) -> T {
unsafe { *self.0.get() }
}
pub struct ReadOnlyCell<T: ?Sized>(cell::UnsafeCell<T>);

impl<T: ?Sized> ReadOnlyCell<T> {
/// Returns a pointer to the current value.
#[inline]
pub fn as_ptr(&self) -> *const T {
self.0.get()
}

/// Converts `&T` to `&ReadOnlyCell<T>`.
#[inline]
pub fn from_ref(t: &T) -> &ReadOnlyCell<T> {
// Safety: ReadOnlyCell is repr(transparent), and &ReadOnlyCell<T> is strictly less capable
// than &T.
unsafe { &*(t as *const T as *const ReadOnlyCell<T>) }
}
Comment on lines +653 to +659
Copy link
Member

@davidhewitt davidhewitt Nov 17, 2023

Choose a reason for hiding this comment

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

This... is interesting. Initially I thought that T: ?Sized is incorrect, because my intuition was that UnsafeCell<str> (for example) is not meaningful. But, it looks like it is allowed, and I guess it's a way of denoting that the underlying str contents may have interior mutability?

I wonder, a longstanding ergonomics question for PyO3 buffers has been the fact that as_slice returns Option<&[ReadOnlyCell<T>]> as a slice-of-cells. I wonder, should we instead be returning Option<&ReadOnlyCell<[T]>> and adding some helpful slice operations over the top?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah some parallel to the standard library's https://doc.rust-lang.org/std/cell/struct.Cell.html#method.as_slice_of_cells might make sense here.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I think that furthers the case that really PyBuffer's slices should be Cell<[T]> / ReadOnlyCell<[T]>.


/// Converts `&Cell<T>` to `&ReadOnlyCell<T>`.
#[inline]
pub fn from_cell(t: &cell::Cell<T>) -> &ReadOnlyCell<T> {
// Safety: Cell and ReadOnlyCell are both repr(transparent), and &ReadOnlyCell<T> is
// strictly less capable than &Cell<T>.
unsafe { &*(t as *const cell::Cell<T> as *const ReadOnlyCell<T>) }
}
}

impl<T: Copy> ReadOnlyCell<T> {
/// Returns a copy of the current value.
#[inline]
pub fn get(&self) -> T {
unsafe { *self.0.get() }
}
}

macro_rules! impl_element(
Expand Down Expand Up @@ -686,7 +704,7 @@ impl_element!(f64, Float);

#[cfg(test)]
mod tests {
use super::PyBuffer;
use super::{PyBuffer, ReadOnlyCell};
use crate::ffi;
use crate::Python;

Expand Down Expand Up @@ -942,4 +960,25 @@ mod tests {
assert_eq!(buffer.to_fortran_vec(py).unwrap(), [10.0, 11.0, 12.0, 13.0]);
});
}

#[test]
fn test_read_only_cell_conversions() {
let mut x = 42;
let x_ref_mut = &mut x;

let x_ref = &*x_ref_mut;
let x_rocell1 = ReadOnlyCell::from_ref(x_ref);
let x_rocell2 = ReadOnlyCell::from_ref(x_ref);
assert_eq!(x_rocell1.get(), x_rocell2.get());
assert_eq!(x_rocell1.get(), 42);

let x_cell = std::cell::Cell::from_mut(x_ref_mut);
let x_rocell3 = ReadOnlyCell::from_cell(x_cell);
let x_rocell4 = ReadOnlyCell::from_cell(x_cell);
assert_eq!(x_rocell3.get(), x_rocell4.get());
assert_eq!(x_rocell3.get(), 42);

// Importantly, this doesn't compile, because x_ref and x_cell cannot coexist.
// _ = *x_ref;
}
}
Loading