-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Swap all utility code types to be heap-types/type-specs #6634
base: master
Are you sure you want to change the base?
Conversation
@@ -351,90 +348,6 @@ static PyType_Spec __pyx_AsyncGenType_spec = { | |||
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_HAVE_FINALIZE, /*tp_flags*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if I should add Py_TPFLAGS_IMMUTABLETYPE
to all of these? That's probably the main visible difference between the heap-type and static-type version, and think they genuinely should be immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try it. Sounds right.
Cython/Utility/ModuleSetupCode.c
Outdated
// "Py_am_send" requires Py3.10 when using type specs. | ||
#define __PYX_HAS_PY_AM_SEND (!CYTHON_USE_TYPE_SPECS || CYTHON_USE_AM_SEND && __PYX_LIMITED_VERSION_HEX >= 0x030A0000) | ||
// "Py_am_send" requires Py3.10 when using type specs (which utility code types do). | ||
#define __PYX_HAS_PY_AM_SEND CYTHON_USE_AM_SEND && __PYX_LIMITED_VERSION_HEX >= 0x030A0000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the static type implementation of coroutines for Py3.[89] then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can be made to with with heaptypes (outside the limited API). It's probably just a case of rewriting tp_as_async
after creation so it points back to a statically allocated version. I'll give that a go in the near future
Cython/Utility/ModuleSetupCode.c
Outdated
// "Py_am_send" requires Py3.10 when using type specs (which utility code types do). | ||
#define __PYX_HAS_PY_AM_SEND CYTHON_USE_AM_SEND && __PYX_LIMITED_VERSION_HEX >= 0x030A0000 | ||
#if !CYTHON_USE_AM_SEND | ||
#define __PYX_HAS_PY_AM_SEND 0 | ||
#elif __PYX_LIMITED_VERSION_HEX >= 0x030A0000 | ||
#define __PYX_HAS_PY_AM_SEND 1 | ||
#else | ||
#define __PYX_HAS_PY_AM_SEND 2 // our own backported implementation | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this means that a Cython module built for Limited API 3.9 is not compatible with one built for Limited API 3.10, right? (Assuming they use generators/coroutines.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Not compatible" in the sense that they shouldn't share the same ABI module.
They don't currently have a separate ABI prefix and probably should
Based on
cython/Cython/Utility/ModuleSetupCode.c
Lines 1237 to 1245 in bf4e9a3
#if CYTHON_COMPILING_IN_LIMITED_API | |
// The limited API makes some significant changes to data structures, so we don't | |
// want to share the implementations compiled with and without the limited API. | |
#if CYTHON_METH_FASTCALL | |
#define __PYX_LIMITED_ABI_SUFFIX "limited_fastcall" | |
#else | |
#define __PYX_LIMITED_ABI_SUFFIX "limited" | |
#endif | |
#else |
I think the incompatibility predates this PR, but it's probably the time to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a different ABI suffix for this.
Thinking about it more, I'm not convinced it's really a problem - I think all that will happen is that the slot won't be set and set won't get used. But it's a little hard to be absolutely sure so I've added the suffix.
Closes #6633 (assuming that it doesn't break anything)