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

implement PySequenceMethods #3535

Closed
wants to merge 2 commits into from
Closed
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
15 changes: 15 additions & 0 deletions src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ use std::mem::{self, ManuallyDrop};
use std::ops::Deref;
use std::ptr::NonNull;

pub(crate) mod ffi_call;
pub(crate) use ffi_call::FromFfiCallResult;

/// Types that are built into the Python interpreter.
///
/// PyO3 is designed in a way that all references to those types are bound
Expand Down Expand Up @@ -64,6 +67,18 @@ impl<'py> Py2<'py, PyAny> {
) -> PyResult<Self> {
Py::from_owned_ptr_or_err(py, ptr).map(|obj| Self(py, ManuallyDrop::new(obj)))
}

/// Internal helper to make an ffi call which produces an owned reference or
/// NULL on error, doing an unchecked downcast to the result type T.
pub(crate) unsafe fn do_ffi_call<R, T>(
&self,
call: impl FnOnce(*mut ffi::PyObject) -> R,
) -> PyResult<T>
where
T: FromFfiCallResult<'py, R>,
{
T::from_ffi_call_result(self.py(), call(self.as_ptr()))
}
}

impl<'py, T> Py2<'py, T> {
Expand Down
59 changes: 59 additions & 0 deletions src/instance/ffi_call.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
use std::os::raw::c_int;

use crate::err::{error_on_minusone, SignedInteger};
use crate::types::any::PyAnyMethods;
use crate::{ffi, Py2, PyErr, PyResult, Python};

/// Internal helper to convert raw ffi call results such as pointers
/// or integers into safe wrappers.
///
/// `unsafe` to implement because it is highly likely this trait is
/// passed a pointer, and is free to do interpret it as it sees fit.
pub(crate) unsafe trait FromFfiCallResult<'py, RawResult>: Sized {
fn from_ffi_call_result(py: Python<'py>, raw: RawResult) -> PyResult<Self>;
Comment on lines +12 to +13
Copy link
Member Author

Choose a reason for hiding this comment

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

Staring at this again I wonder if it should have been a safe trait with the method being unsafe fn from_ffi_call_result. Both are a bit awkward because only the implementation for *mut ffi::PyObject actually needs to be unsafe. Anyway, this is only an internal API so it doesn't need to be perfect...

Copy link
Member

Choose a reason for hiding this comment

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

Reading the comment above, why does it have to be unsafe at all? "May get a pointer and do unsafe stuff with it" doesn't sound like a strong argument :)

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'm generally of the understanding that the pointer alone can't communicate that's it's safe to read.

Probably the right approach is to combine this with some wrapper along the lines of #1809 so that the trait doesn't need to be unsafe at all. I'll keep playing.

}

/// For Py2<T>, perform an unchecked downcast to the target type T.
unsafe impl<'py, T> FromFfiCallResult<'py, *mut ffi::PyObject> for Py2<'py, T> {
fn from_ffi_call_result(py: Python<'py>, raw: *mut ffi::PyObject) -> PyResult<Self> {
unsafe { Py2::from_owned_ptr_or_err(py, raw).map(|any| any.downcast_into_unchecked()) }
}
}

unsafe impl<'py, T> FromFfiCallResult<'py, T> for ()
where
T: SignedInteger,
{
fn from_ffi_call_result(py: Python<'py>, raw: T) -> PyResult<Self> {
error_on_minusone(py, raw)
}
}

unsafe impl<'py, T> FromFfiCallResult<'py, T> for T
where
T: SignedInteger,
{
fn from_ffi_call_result(py: Python<'py>, raw: T) -> PyResult<Self> {
if raw != T::MINUS_ONE {
Ok(raw)
} else {
Err(PyErr::fetch(py))
}
}
}

unsafe impl<'py> FromFfiCallResult<'py, c_int> for bool {
fn from_ffi_call_result(py: Python<'py>, raw: c_int) -> PyResult<Self> {
match raw {
0 => Ok(false),
1 => Ok(true),
_ => Err(PyErr::fetch(py)),
}
}
}

/// Convert an isize which is known to be positive to a usize.
#[inline]
pub(crate) fn positive_isize_as_usize(x: isize) -> usize {
x as usize
}
1 change: 1 addition & 0 deletions src/prelude.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@ pub use crate::wrap_pyfunction;
// Expected to become public API in 0.21
// pub(crate) use crate::instance::Py2; // Will be stabilized with a different name
// pub(crate) use crate::types::any::PyAnyMethods;
// pub(crate) use crate::types::sequence::PySequenceMethods;
2 changes: 1 addition & 1 deletion src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ mod notimplemented;
mod num;
#[cfg(not(PyPy))]
mod pysuper;
mod sequence;
pub(crate) mod sequence;
pub(crate) mod set;
mod slice;
mod string;
Expand Down
Loading