GH-90699: use statically allocated interned strings in typeobject's slotdefs #94706
Conversation
kumaraditya303
commented
Jul 9, 2022
•
- Issue: gh-90699
kumaraditya303
added
skip news
|
If you want to schedule another build, you need to add the " |
bedevere-bot
removed
the
|
Webassembly buildbots failures are unrelated. |
This is great! I literally wrote a note to myself yesterday to do this, so thanks for noticing and picking this up.
There's really only one thing I'd suggest you change.
| #define TPSLOT(NAME, SLOT, FUNCTION, WRAPPER, DOC, NAMEOBJ) \ | ||
| {NAME, offsetof(PyTypeObject, SLOT), (void *)(FUNCTION), WRAPPER, \ | ||
| PyDoc_STR(DOC)} | ||
| #define FLSLOT(NAME, SLOT, FUNCTION, WRAPPER, DOC, FLAGS) \ | ||
| PyDoc_STR(DOC), .name_strobj = NAMEOBJ} |
There was a problem hiding this comment.
The extra macro argument is unnecessary (and the duplication is avoidable):
#define TPSLOT(NAME, SLOT, FUNCTION, WRAPPER, DOC) \
{#NAME, offsetof(PyTypeObject, SLOT), (void *)(FUNCTION), WRAPPER, \
PyDoc_STR(DOC), .name_strobj = &_Py_ID(NAME)}(You could also use _Py_XSTRINGIFY() from pymacro.h.)
Note that this requires that the quotation marks be dropped in all the uses of this macro. I'll leave a separate comment to show an example.
There was a problem hiding this comment.
One consequence of this change is that Tools/scripts/generate_global_objects.py cannot automatically detect these names. So the script would need to be updated. This includes making sure all the names are in the IDENTIFIERS list at the top of the file. I think this is worth doing.
There was a problem hiding this comment.
FYI, I opened gh-94715 to separately consider the idea of detecting the indirect uses of _Py_ID() as well. That can be dealt with in a separate PR (if desirable), though.
| TPSLOT("__getattr__", tp_getattr, NULL, NULL, ""), | ||
| TPSLOT("__setattr__", tp_setattr, NULL, NULL, ""), | ||
| TPSLOT("__delattr__", tp_setattr, NULL, NULL, ""), | ||
| TPSLOT("__getattribute__", tp_getattr, NULL, NULL, "", &_Py_ID(__getattribute__)), |
There was a problem hiding this comment.
| TPSLOT("__getattribute__", tp_getattr, NULL, NULL, "", &_Py_ID(__getattribute__)), | |
| TPSLOT(__getattribute__, tp_getattr, NULL, NULL, ""), |
There was a problem hiding this comment.
I am not sure about this, the regex currently searches for _Py_ID(...) so it will not pick this up.
There was a problem hiding this comment.
Yeah, see #94706 (comment).
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
