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

Conversation

davidhewitt
Copy link
Member

For #3382

This moves the API for PySequence to be implemented by a pool-free implementation. As per the rest of the PRs, the new trait PySequenceMethods is restricted to pub(crate) for now while we're iterating on this.

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label Oct 20, 2023
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 20, 2023

CodSpeed Performance Report

Merging #3535 will not alter performance

Comparing davidhewitt:sequence2 (1583828) with main (d8a6f37)

Summary

✅ 78 untouched benchmarks


#[inline]
fn get_item(&self, index: usize) -> PyResult<Py2<'py, PyAny>> {
unsafe {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to abstract out the repeating

Py2::from_owned_ptr_or_err(
    self.py(),
    f(self.as_ptr()),
)
.map(|py2| py2.downcast_into_unchecked())

using a for example a closure-based API with f: FnOnce(*mut PyObject) -> R?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think there could definitely be a pub(crate) helper to do this. I'll have a play 👍

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've pushed a commit which adds Py2::do_ffi_call which has a generic return type with a bit of trait magic to map raw ffi return values into various safe wrappers. What do you think?

src/types/sequence.rs Outdated Show resolved Hide resolved
Comment on lines +12 to +13
pub(crate) unsafe trait FromFfiCallResult<'py, RawResult>: Sized {
fn from_ffi_call_result(py: Python<'py>, raw: RawResult) -> PyResult<Self>;
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.

@davidhewitt
Copy link
Member Author

Closing in favour of #3572

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants