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-101714: Isolate _curses extension module #104732

Closed
wants to merge 15 commits into from

Conversation

chgnrdv
Copy link
Contributor

@chgnrdv chgnrdv commented May 22, 2023

#101714

  • converted PyCursesWindowObject to heap type
  • added module state containing previously global PyCursesWindow_Type, PyCursesError and screen_encoding; added get__curses_state and find__curses_state_by_type functions; changed functions and methods to access members of state or take state as a parameter
  • got rid of ModDict global variable, made functions/methods access it through PyModule_GetDict call; changed SetDictInt to accept dict as parameter
  • converted module to multi-phase init
  • changed signatures of functions used by C API in order to make exception stored in module state accessible by API users; reworked macros in Include/py_curses.h to reflect these changes
  • added clinic_state() macro for access to PyCursesWindow_Type in AC-generated code

chgnrdv added 2 commits May 22, 2023 02:37
* converted `PyCursesWindowObject` to heap type
* added module state containing previously global `PyCursesWindow_Type`, `PyCursesError` and `screen_encoding`; added `get__curses_state` and `find__curses_state_by_type` functions; changed functions and methods to access members of state or take state as a parameter
* got rid of `ModDict` global variable, made functions/methods access it through `PyModule_GetDict` call; changed `SetDictInt` to accept dict as parameter
* converted module to multi-phase init
* changed signatures of functions used by C API in order to make exception stored in module state accessible by API users; reworked macros in Include/py_curses.h to reflect these changes
* added `clinic_state()` macro for access to `PyCursesWindow_Type` in AC-generated code
@chgnrdv
Copy link
Contributor Author

chgnrdv commented May 22, 2023

@erlend-aasland @kumaraditya303
There are still some things I have doubts about, e.g. how I worked around necessity to expose PyCursesError to C API users, e. g. _curses_panel module. I see that it could be done in more pretty way through capsule import, in the same manner as between _elementtree and pyexpat, but I'm not sure about it being non-breaking change.

chgnrdv and others added 3 commits May 23, 2023 02:23
* added `__reduce__` method to `_curses.window` type to make it non-pickable, as its static version was
* added tests to check if `_curses.window` is non-instantiable/non-picklable/immutable
* moved `SetDictInt` macro from function body
* added NULL check for value returned by `PyCursesWindow_New` in `_curses_initscr_impl`
@chgnrdv chgnrdv marked this pull request as ready for review May 23, 2023 00:01
@chgnrdv
Copy link
Contributor Author

chgnrdv commented May 23, 2023

@erlend-aasland @kumaraditya303 for review.

@erlend-aasland
Copy link
Contributor

I'm sorry, but I don't have the bandwidth to review this in the near future.

@chgnrdv
Copy link
Contributor Author

chgnrdv commented Jun 2, 2023

@erlend-aasland, no problem, these aren't first priority changes anyway :)

/* Definition of exception curses.error */

static PyObject *PyCursesError;
/*[clinic end generated code: output=da39a3ee5e6b4b0d input=83369be6e20ef0da]*/

/* Tells whether setupterm() has been called to initialise terminfo. */
static int initialised_setupterm = FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

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

These would also have to be moved to module state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Not sure if I did it the best way though.

Copy link
Contributor Author

@chgnrdv chgnrdv Jun 16, 2023

Choose a reason for hiding this comment

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

Sure I didn't. At least exposing parts of _curses module state to C API in this way doesn't look safe for me.

chgnrdv and others added 3 commits June 16, 2023 20:46
…cr`/`start_color` funcs were called, to module state

* removed funcs that were exposed to curses C API and used vars mentioned above
* made C API use pointers to corresponding fields in curses state
@chgnrdv chgnrdv requested a review from kumaraditya303 June 16, 2023 23:24
@erlend-aasland
Copy link
Contributor

@chgnrdv: Sorry, but the linked issue has been closed as wont-fix. It is unfortunate given you spent a lot of time working on this. On the bright side: hopefully, you learnt a lot about the C API, extension modules and how to isolate them! Thank you for your interest in improving CPython.

@chgnrdv
Copy link
Contributor Author

chgnrdv commented Jul 6, 2023

@erlend-aasland, no problem, I was ready for this as I read the conversation under original issue and knew that the need to isolate this module is debatable :)

On the bright side: hopefully, you learnt a lot about the C API, extension modules and how to isolate them!

Sure! Thank you for opportunity :)
I'll see if I can help with one of remaining non-isolated modules.

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.

4 participants