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

Get some minimal cdef classes working with Py_LIMITED_API #5617

Merged

Conversation

da-woods
Copy link
Contributor

(A good chunk of these changes are actually to do with the auto-generated pickling functions for an empty class)

@da-woods da-woods changed the title Get some minimal cdef classes worth with Py_LIMITED_API Get some minimal cdef classes working with Py_LIMITED_API Aug 14, 2023
Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Looks good to me overall.

@@ -6582,6 +6585,8 @@ def attribute_is_likely_method(attr):
code.putln("%s = 1;" % arg_offset_cname)
code.putln("}")
code.putln("}")
code.putln("#endif") # CYTHON_UNPACK_METHODS
# TODO may need to deal with unused variables in the #else case
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's why I used a simple if statement instead of #if. At least we're not creating section specific temps here, so it should be possible to fake the variable usage.

Comment on lines 3417 to 3423
code.putln("#if !CYTHON_USE_TYPE_SLOTS && !CYTHON_COMPILING_IN_PYPY")
# Asking for PyType_GetSlot(..., Py_tp_free) seems to cause an error in pypy
code.putln("freefunc tp_free = (freefunc)PyType_GetSlot(Py_TYPE(o), Py_tp_free);")
code.putln("if (tp_free) tp_free(o);")
code.putln("#else")
code.putln("(*Py_TYPE(o)->tp_free)(o);")
code.putln("#endif")
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to spell both implementations, here and above, in the same order. I'd reverse the conditions here, not above. It seems more natural to spell out the specific cases followed by the generic fallback.

Cython/Utility/ExtensionTypes.c Show resolved Hide resolved
#define CYTHON_USE_TP_FINALIZE 1
#define CYTHON_USE_TP_FINALIZE 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm … does this not work with PyType_GetSlot()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's PyObject_CallFinalizerFromDealloc. I don't think we can easily replace that using the tools in the limited API, but I'll add an explanatory comment here so it's at least clear what's missing.

@scoder
Copy link
Contributor

scoder commented Aug 24, 2023 via email

@scoder scoder added this to the 3.0.1 milestone Aug 24, 2023
@scoder scoder merged commit b551a50 into cython:master Aug 24, 2023
76 checks passed
@scoder
Copy link
Contributor

scoder commented Aug 24, 2023

Nice!

@da-woods da-woods deleted the limited-api-with-py-limited-api-cdef-clas branch August 24, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants