Change CyFunction to compile with Py_LIMITED_API #5556
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mostly that we store the PyCFunctionObject as an attribute of it rather than "inheriting" from PyCFunctionObject.
da-woods
changed the title
|
Quick comment (this is great work overall), I don't think we discard the comment in
#else // CYTHON_COMPILING_IN_LIMITED_API
We should be using C89 here.
|
|
+#if !CYTHON_COMPILING_IN_LIMITED_API
Py_CLEAR(((PyCFunctionObject*)m)->m_module);
+#else
+ Py_CLEAR(m->func);
+#endif
If there are alternative implementations, it's usually better to put the special case first, with a positive macro guard. If we add more special cases at some point, we'll end up with mixed positive and negative guards, and would probably reverse the condition at that point. So why not avoid that risk of future churn all together.
|
scoder
reviewed
Jul 23, 2023
|
There's a few bits of the destructor which I haven't got quite right... Don't merge yet! |
|
Ok. Anyway, LGTM now. Even the ABI module change is fine, because it only touches modules that use the still officially experimental support for the Limited C-API. |
|
Got to the bottom of my referencing counting problems with this. I'll merge it as soon as the tests pass. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mostly that we store the PyCFunctionObject as an attribute of it rather than "inheriting" from PyCFunctionObject.
(Actually testing the compilation with
Py_LIMITED_APIdefined needs to it be put on top of #5550, but I think this PR can stand alone generally)