Skip to content

Split the current "CYTHON_COMPILING_IN_LIMITED_API" macro guard#3611

Merged
scoder merged 91 commits intomasterfrom
clean_up_capi_features
May 25, 2021
Merged

Split the current "CYTHON_COMPILING_IN_LIMITED_API" macro guard#3611
scoder merged 91 commits intomasterfrom
clean_up_capi_features

Conversation

@scoder
Copy link
Contributor

@scoder scoder commented May 14, 2020

Use separate feature guards for:

  1. using a global module state struct ("CYTHON_USE_MODULE_STATE")
  2. using PyType_FromSpec() for extension types ("CYTHON_USE_TYPE_SPECS")
  3. actual limited-API special casing ("CYTHON_COMPILING_IN_LIMITED_API")

@scoder scoder added this to the 3.0 milestone May 14, 2020
@scoder scoder force-pushed the clean_up_capi_features branch 4 times, most recently from 828223e to addf112 Compare May 18, 2020 11:50
scoder added 15 commits May 20, 2020 09:10
…separate feature guards:

1) using a global module state struct ("CYTHON_USE_MODULE_STATE")
2) using PyType_FromSpec() for extension types ("CYTHON_USE_TYPE_FROM_SPEC")
3) actual limited-API special casing ("CYTHON_COMPILING_IN_LIMITED_API")
… we are not compiling against the Limited-API.
…top relying on the PyTypeObject struct being available in several places by using the type pointer instead (and setting it early enough).

Rename "CYTHON_USE_TYPE_FROM_SPEC" feature flag to "CYTHON_USE_TYPE_SPECS".
#if CYTHON_USE_TYPE_SPECS && PY_VERSION_HEX >= 0x03080000
// In Py3.8+, instances of heap types need to decref their type on deallocation.
// https://bugs.python.org/issue35810
#define __Pyx_PyHeapTypeObject_GC_Del(obj) { \
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that you should also include the heap types in tp_traverse and tp_clear? (https://bugs.python.org/issue42972)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Erm, yes… I hadn't thought about that yet, but sure, it's a live reference that can participate in reference cycles, any time.
Good call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the relevant change: python/cpython@1cf15af
(note that it contains a removal of a previous change that tried a generic implementation).

@mattip
Copy link
Contributor

mattip commented May 25, 2021

It seems that when using CYTHON_USE_TYPE_SPECS fails tests, but the rest is passing. Would it be reasonable to split that out as a separate PR?

@scoder
Copy link
Contributor Author

scoder commented May 25, 2021

The only remaining issue with the "type specs" tests is that profiling apparently counts cpdef functions twice, for wrapper and underlying function. That suggests a bug in Cython.

But I agree that we don't have to wait for that. The overall changes are valuable already.

@scoder
Copy link
Contributor Author

scoder commented May 25, 2021

There's still #3611 (comment) to be resolved. Heap types need GC support.

@scoder scoder merged commit 70be8d1 into master May 25, 2021
@scoder
Copy link
Contributor Author

scoder commented May 25, 2021

Merged. Finally. Let's see how many PRs this breaks. :)

@scoder scoder deleted the clean_up_capi_features branch May 25, 2021 15:19
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.

4 participants