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

ffi cleanup: abstract.h #1436

Merged
merged 6 commits into from
Feb 28, 2021
Merged

ffi cleanup: abstract.h #1436

merged 6 commits into from
Feb 28, 2021

Conversation

nw0
Copy link
Contributor

@nw0 nw0 commented Feb 19, 2021

  • This PR makes PyIter_Check available to Python earlier than 3.8 under the limited API.
    This was due to some interaction between the cfg(not(Py_LIMITED_API) on ffi::cpython, and the cfg on PyIter_Check within the ffi::cpython module.
    However, it's always been in the limited API: as a macro before 3.8, and as a function since 3.8. Under the non-limited API it's always a macro.

  • Consequently we can implement PyTryFrom for PyIterator for all versions. I've (speculatively) commented out the cfg preventing that.

  • Similar changes for PyIndex_Check.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Good catch 👍🏼 , thank you!

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

👍 this is a great catch, thanks!

@davidhewitt
Copy link
Member

I'm going to rebase this, force-push and merge shortly. Thanks again.

@davidhewitt
Copy link
Member

Hmm, after rebasing we still have issues: the "macro" forms of PyIter_Check and PyIndex_Check use the contents of the type object, which isn't available with the limited api...

@nw0
Copy link
Contributor Author

nw0 commented Feb 23, 2021

Hmm, after rebasing we still have issues: the "macro" forms of PyIter_Check and PyIndex_Check use the contents of the type object, which isn't available with the limited api...

Ouch. I've double-checked and PyIter_Check in 3.7 tries to use the contents of the opaque PyTypeObject, even under the limited API. I wonder why nobody noticed?

I think the best thing to do here is to remove the functions again :( will do this by the weekend.

While these are defined as macros in the Python C API, they rely on
access to the PyTypeObject structure, which is not part of the limited
API for those versions.
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks for finishing this off.

Comment on lines +92 to +96
// Defined as this macro in Python 3.6, 3.7 limited API, but relies on
// non-limited PyTypeObject. Don't expose this since it cannot be used.
#[cfg(not(any(Py_LIMITED_API, PyPy)))]
#[inline]
pub unsafe fn PyIter_Check(o: *mut PyObject) -> c_int {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@davidhewitt davidhewitt merged commit 593be05 into PyO3:master Feb 28, 2021
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