Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-123961: move PyCursesError in _cursesmodule.c to a global state variable#124729
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
gh-123961: move PyCursesError in _cursesmodule.c to a global state variable #124729
Uh oh!
There was an error while loading. Please reload this page.
Conversation
picnixz commented Sep 28, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
_cursesmodules.c a multi-phase initialization module_cursesmodule.c a multi-phase initialization modulepicnixz commented Sep 28, 2024
I don't know whether I should include a NEWS entry or not. One entry could be that I actually fixed the reference leaks in the curses module. However, I want to know that what I did would not cause an issue with the exported C API (I don't want a segfault for this one and I would like to be sure that what needs to be process-wide is indeed process-wide and not stored in the module's state). |
bedevere-bot commented Sep 28, 2024
picnixz commented Sep 28, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Ok, so |
picnixz commented Sep 28, 2024
Note: for now, I only used the |
_cursesmodule.c a multi-phase initialization modulePyCursesError in _cursesmodule.c to a global state variableUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…cpython into curses/multiphase-module-123961
picnixz commented Sep 29, 2024
If I were to also change Ideally, This will also require an additional helper: staticinline_cursesmodule_state*get_cursesmodule_state_by_cls(PyObject*Py_UNUSED(cls)){return&curses_global_state} |
picnixz commented Sep 29, 2024
I don't think I can use more the state than what is being currently used. |
vstinner left a comment
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.
You may rename st to state.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner left a comment
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
UPDATE: managed to make it smaller.
Previous discussion (now resolved)
Ideally, I wanted to make multiple PRs for the curses module but simply adding a module state with the minimal logic would not be helpful at all. So I decided to do the following in one go:
The reason I couldn't find a way to split those changes is due to the exposed C API. The C API uses parameter-less functions and thus does not have access to the module's state. More precisely, we need to access the exception type in three different contexts without relying on a global variable (the purpose is to remove the global variables):
There is a way to split the PR into two, namely by getting
PyCursesErrorvia an import. Informally, we would consider the module's dict as the state (which we must do for parameter-less functions) though I honestly think it's not worth that effort for window and module's methods.Alternatively, we could first make the module a multi-phase initialization module, without any state and keep the global variables. However, I'm not sure that's a good idea. Note that I cleanly separated the commits so that I can pick them up if needed.
Finally, I did not apply any cosmetic changes even though I was very tempted. The only cosmetic change I did was for the macros so that they readability is improved and if I'm anyway editing the lines. If a cosmetic change slipped through, please tell me.
_cursesmodule.cto fix reference leaks #123961