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

pymethods: support buffer protocol #2066

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

davidhewitt
Copy link
Member

For #1884

Implements the buffer protocol via two new methods __getbuffer__ and __releasebuffer__.

They are unsafe because they operate on ffi::Py_buffer directly. I considered making a "safe" API, however also at this point in time I think it's better if we just get the migration away from #[pyproto] complete. We can revisit a safe design later if needed.

Cython implements these methods exactly the same way. https://cython.readthedocs.io/en/latest/src/userguide/buffer.html

@davidhewitt davidhewitt force-pushed the buffer-pymethods branch 2 times, most recently from 4bda612 to 630b493 Compare December 21, 2021 07:31
@birkenfeld
Copy link
Member

I'm wondering if the method needs to be unsafe. Unsafety already needs to be used internally to fill the Py_buffer* fields.

@mejrs
Copy link
Member

mejrs commented Dec 21, 2021

I'm wondering if the method needs to be unsafe. Unsafety already needs to be used internally to fill the Py_buffer* fields.

They need to be unsafe if these functions are callable from Rust.

@davidhewitt
Copy link
Member Author

They need to be unsafe if these functions are callable from Rust.

Exactly.

For the sake of putting it out there, I looked at a couple of alternatives:

  • These functions taking &mut ffi::Py_buffer instead of *mut. I don't think that's actually sound; in the general case running any Python APIs inside these functions could theoretically cause something else to modify the buffer structure (even if in practice it probably won't happen).
  • __getbuffer__ could be made safe if it returned ffi::Py_buffer instead. Unfortunately this same idea can't be applied to __releasebuffer__, as it's not producing a new ffi::Py_buffer.
  • __releasebuffer__ could probably be made safe by having some kind of safe PyBufferInfo wrapper which doesn't hold references for indeterminate length of time. It seemed like a lot of work so I felt it was easier to just keep to the ffi structures for now.

@davidhewitt
Copy link
Member Author

I'll give this a few more days in case anyone has further concerns and / or ideas, otherwise I'll merge this as-is in order to make progress towards 0.16.

@davidhewitt davidhewitt merged commit 20d1139 into PyO3:main Jan 3, 2022
@davidhewitt davidhewitt deleted the buffer-pymethods branch January 3, 2022 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants