Skip to content
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

bpo-1635741: Port symtable module to multiphase initialization #23361

Merged
merged 2 commits into from
Nov 18, 2020

Conversation

tiran
Copy link
Member

@tiran tiran commented Nov 18, 2020

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue1635741

Automerge-Triggered-By: GH:tiran

Signed-off-by: Christian Heimes <christian@python.org>
@tiran tiran changed the title Port symtable module to multiphase initialization bpo-1635741: Port symtable module to multiphase initialization Nov 18, 2020
static int
symtable_init_stentry_type(PyObject *m)
{
return PyType_Ready(&PySTEntry_Type);
Copy link
Member

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?

Copy link
Member Author

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.

Modules/symtablemodule.c Outdated Show resolved Hide resolved

static int
symtable_init_constants(PyObject *m)
{
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@miss-islington miss-islington merged commit c701101 into python:master Nov 18, 2020
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…nGH-23361)

Signed-off-by: Christian Heimes <christian@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants