gh-149044: Implement PEP 820 – PySlot: Unified slot system for the C API#149055
gh-149044: Implement PEP 820 – PySlot: Unified slot system for the C API#149055encukou wants to merge 48 commits intopython:mainfrom
Conversation
bedevere-bot
removed
the
🔨 test-with-buildbots
encukou
requested review from
a team,
AA-Turner,
AlexWaygood,
FFY00,
ZeroIntensity,
brettcannon,
corona10,
emmatyping,
ericsnowcurrently,
erlend-aasland,
hugovk,
kumaraditya303,
markshannon,
ncoghlan and
warsaw
as code owners
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
Documentation looks pretty good. I haven't looked at all of the C code yet.
Co-authored-by: Peter Bierma <zintensitydev@gmail.com>
Documentation build overview
72 files changed ·
|
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
I wrote FFI bindings for this PR for PyO3 this afternoon and spotted several minor issues.
| #define PySlot_PTR(NAME, VALUE) \ | ||
| {(NAME), PySlot_INTPTR, {0}, {(void*)(VALUE)}} | ||
|
|
||
| #define PySlot_PTR_STATIC(NAME, VALUE) \ | ||
| {(NAME), PySlot_INTPTR | PySlot_STATIC, {0}, {(void*)(VALUE)}} |
There was a problem hiding this comment.
Can you spell these last two with the explicit names of the struct fields like the ones above? Also while you're touching this code, IMO these two definitions should come before PySlot_END.
There was a problem hiding this comment.
These macros are for code compatible with C++11 and below, which doesn't support designated initializers (that is, the explicit names).
So, both style issues are intentional :) I added a comment.
|
|
||
| #define PySlot_OPTIONAL 0x01 | ||
| #define PySlot_STATIC 0x02 | ||
| #define PySlot_INTPTR 0x04 |
There was a problem hiding this comment.
nit: since these are u16 constants, consider typsetting them as e.g. 0x0001 like you did for Py_slot_invalid below
| #define PySlot_STATIC 0x02 | ||
| #define PySlot_INTPTR 0x04 | ||
|
|
||
| #define Py_slot_invalid 0xffff |
There was a problem hiding this comment.
for consistency should this be called Py_slot_INVALID maybe?
There was a problem hiding this comment.
That would be consistent with the wrong thing. Slot IDs are named Py_tp_repr, Py_mod_exec, etc. Flags are a new thing, so they follow the current style guide: mixed prefix + uppercase.
| #define _Py_SLOT_COMPAT_VALUE(OLD, NEW) OLD | ||
| #endif | ||
|
|
||
| #define Py_slot_end 0 |
There was a problem hiding this comment.
Maybe worth adding a comment above these that they're all u16 values?
There was a problem hiding this comment.
In C, the preprocessor macros are just numbers :)
In PyType_Slot, these values are used as int.
| #define Py_TP_USE_SPEC NULL | ||
| #endif | ||
| #if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= _Py_PACK_VERSION(3, 15) | ||
| PyAPI_FUNC(PyObject *) PyType_FromSlots(struct PySlot *); |
There was a problem hiding this comment.
This is missing the argument name, the docs call it slots.
| #define Py_tp_finalize 80 | ||
| #define Py_am_send 81 | ||
| #define Py_tp_vectorcall 82 | ||
| #define Py_tp_token 83 |
There was a problem hiding this comment.
Everything through here is duplicated with typeslots.h. Was that intentional? It probably makes sense for there to be only one source of truth for these constants.
There was a problem hiding this comment.
Thanks for the catch! I seem to have dropped the commit that removes the unused file.
| #ifndef _Py_HAVE_SLOTS_H | ||
| #define _Py_HAVE_SLOTS_H | ||
|
|
||
| typedef void (*_Py_funcptr_t)(void); |
There was a problem hiding this comment.
PyO3 needs to wrap this type directly so you might as well give this a public non-underscored name.
There was a problem hiding this comment.
Can PyO3 use the underlying type, or better yet, some “generic C function pointer” type?
4 participants
📚 Documentation preview 📚: https://cpython-previews--149055.org.readthedocs.build/