gh-123961: move PyCursesError in _cursesmodule.c to a global state variable - #124729
gh-123961: move PyCursesError in _cursesmodule.c to a global state variable#124729vstinner merged 39 commits into
Conversation
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
Ok, so test_class leaks (#124722). |
Sorry, something went wrong.
|
Note: for now, I only used the get_cursesmodule_state function in the module's methods but not in the methods for window objects. Windows objects directly access the global state (this is just another global variable) but in the future, when the type will be a heap type, I'll be able to access the underlying module (and convert them to get_cursesmodule_state_by_cls calls). |
Sorry, something went wrong.
…cpython into curses/multiphase-module-123961
|
If I were to also change PyCursesCheckERR so that it accepts a module / state argument, the PR will be larger. Do you want me to that change? Ideally, PyCursesCheckERR(code, name) should be changed into PyCursesCheckERR(module, code, name) and PyCursesCheckERR_ForWin(window, code, name) so that we only retrieve the state when needed. This will also require an additional helper: static inline _cursesmodule_state *
get_cursesmodule_state_by_cls(PyObject *Py_UNUSED(cls))
{
return &curses_global_state;
} |
Sorry, something went wrong.
|
I don't think I can use more the state than what is being currently used. |
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
You may rename st to state.
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
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 PyCursesError via 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.