Skip to content

Conversation

@davidhewitt
Copy link
Member

Follow up to #2481.

This PR changes the PyCapsule API as discussed in that PR. In addition, I realised that error handling in the implementation was potentially incorrect - I had to rework a few things so that in case of an "invalid capsule" Python errors are always either propagated or cleared as I felt was appropriate.

@davidhewitt davidhewitt requested a review from adamreichold June 28, 2022 17:23
Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

I think the changes are correct. I am not sure whether our API matches the intent of the CPython API it is wrapping though.

@davidhewitt davidhewitt requested a review from adamreichold June 29, 2022 05:25
Copy link
Member

@adamreichold adamreichold left a comment

Choose a reason for hiding this comment

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

Would you also change reference() to take name: Option<&CStr> argument, or given that method is unsafe would you leave it with the existing semantics it has here (where issues with validity / name are ignored)?

This is a difficult decision. I think going for the principle of least surprise, I would expect reference to call pointer and just take care of casting and dereferencing, so I would expect an equivalent signature like

pub unsafe fn reference<T>(&self, name: Option<&CStr>) -> PyResult<&T>;

I am also not sure if the currently implementation shouldn't at least clear any Python exceptions even if it ignores them?

But as written before, this is said under the assumption that we should match the CPython API where I do not understand why the CPython API wants to independently verify the name. (Could this be a consistency check to prevent type confusion?)

@davidhewitt davidhewitt mentioned this pull request Jul 3, 2022
7 tasks
@davidhewitt
Copy link
Member Author

After thinking about this for a while, I noticed that pybind11 also deviates from the C API and automatically loads the name, as we were, so I think I will roll back a couple of these changes to be closer to our previous API.

(https://github.com/pybind/pybind11/blob/75007dda72ad4508064c1f080394eae50d3a61ee/include/pybind11/pytypes.h#L1626)

@davidhewitt davidhewitt force-pushed the rework-capsule branch 2 times, most recently from 58ab0eb to a4d4c91 Compare July 21, 2022 21:24
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.

2 participants