-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-1635741: Port symtable module to multiphase initialization #23361
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
Conversation
Signed-off-by: Christian Heimes <christian@python.org>
ee17c63
to
bb7961b
Compare
static int | ||
symtable_init_stentry_type(PyObject *m) | ||
{ | ||
return PyType_Ready(&PySTEntry_Type); |
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 don't see the value of having two exec slots. Why not simply putting all exec code into a single function?
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.
They do different things. Also PySTEntry_Type
is define somewhere else and we might want to move the type initialization when the PySTEntry_Type
is refactored.
|
||
static int | ||
symtable_init_constants(PyObject *m) | ||
{ |
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'm trying to slowly use macros to factorize the code, something like:
#define ADD_INT(value) \
if (PyModule_AddIntConstant(module, #value, value) < 0) { \
return -1; \
}
A more generic machinery like the one you proposed would avoid such macro, and until there is such API, IMO using a macro makes the code more readable:
ADD_INT(USE);
You can add a second macro for PyModule_AddIntConstant() calls.
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'd rather not spend more time then necessary. This was a simple search and replace job.
Let's consider refactoring after PyModuleConstants_Def
from #23286 has landed. The new feature will make module constant definitions easier.
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.
LGTM.
…nGH-23361) Signed-off-by: Christian Heimes <christian@python.org>
Signed-off-by: Christian Heimes christian@python.org
https://bugs.python.org/issue1635741
Automerge-Triggered-By: GH:tiran