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

gh-123961: move PyCursesError in _cursesmodule.c to a global state variable #124729

Merged
merged 39 commits into from
Sep 29, 2024

Conversation

picnixz
Copy link
Contributor

@picnixz picnixz commented Sep 28, 2024

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:

  • Add a module's state that holds the curses exception type and the curses window object type.
  • Make the window object type a heap type.
  • Use multi-phase initialization for the module.

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):

  • In a parameter-less function: we need to re-import the curses module.
  • In a window object's method: if we use a static type, we still can't access the module's state so it does not makes sense to me to make a change where I would first import the exception and in the next PR I would access it directly from the module's state.
  • In a module's method: we can avoid using the state since the exception is stored in the module's dict, but again, this does not make sense to keep a temporary workaround for a few days.

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.

@picnixz picnixz changed the title gh-123961: make _cursesmodules.c a multi-phase initialization module gh-123961: make _cursesmodule.c a multi-phase initialization module Sep 28, 2024
@picnixz
Copy link
Contributor Author

picnixz 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).

@picnixz picnixz added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 28, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 228348c 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Sep 28, 2024
@picnixz
Copy link
Contributor Author

picnixz commented Sep 28, 2024

Ok, so test_class leaks (#124722).

@picnixz
Copy link
Contributor Author

picnixz commented Sep 28, 2024

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).

@picnixz picnixz changed the title gh-123961: make _cursesmodule.c a multi-phase initialization module gh-123961: move PyCursesError in _cursesmodule.c to a global state variable Sep 28, 2024
@picnixz picnixz requested review from vstinner and removed request for ericsnowcurrently September 28, 2024 16:24
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
@picnixz
Copy link
Contributor Author

picnixz commented Sep 29, 2024

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;
}

@picnixz
Copy link
Contributor Author

picnixz commented Sep 29, 2024

I don't think I can use more the state than what is being currently used.

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.

You may rename st to state.

Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
Modules/_cursesmodule.c Outdated Show resolved Hide resolved
@vstinner vstinner enabled auto-merge (squash) September 29, 2024 14:56
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

@vstinner vstinner merged commit 4d8e7c4 into python:main Sep 29, 2024
37 checks passed
@picnixz picnixz deleted the curses/multiphase-module-123961 branch September 29, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants