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-41798: Allocate the _curses._C_API on the heap memory. #24186

Merged
merged 3 commits into from Jan 22, 2021

Conversation

@shihai1991
Copy link
Member

@shihai1991 shihai1991 commented Jan 10, 2021

@@ -4732,9 +4732,28 @@ PyInit__curses(void)
return NULL;
ModDict = d; /* For PyCurses_InitScr to use later */

void **PyCurses_API = PyMem_Calloc(PyCurses_API_pointers, sizeof(void *));

This comment has been minimized.

@vstinner

vstinner Jan 20, 2021
Member

This API is weird. Would you be interested to add a structure of 4 pointers? Something like PyDateTime_CAPI.

The 4 pointers are used by:

#define PyCursesWindow_Type (*(PyTypeObject *) PyCurses_API[0])
#define PyCursesSetupTermCalled  {if (! ((int (*)(void))PyCurses_API[1]) () ) return NULL;}
#define PyCursesInitialised      {if (! ((int (*)(void))PyCurses_API[2]) () ) return NULL;}
#define PyCursesInitialisedColor {if (! ((int (*)(void))PyCurses_API[3]) () ) return NULL;}

Maybe you can name the 4 members as:

PyTypeObject *Window_Type
int SetupTermCalled(void)
int Initialised(void)
int InitialisedColor(void)

Or I can merge this PR and then you work on a new PR to add a structure. As you want, tell me what do you prefer ;-)

This comment has been minimized.

@shihai1991

shihai1991 Jan 21, 2021
Author Member

Or I can merge this PR and then you work on a new PR to add a structure. As you want, tell me what do you prefer ;-)

Oh, I perfer to use another PR to add this structure in this weekend :)

Modules/_cursesmodule.c Outdated Show resolved Hide resolved
c_api_object = PyCapsule_New(PyCurses_API, PyCurses_CAPSULE_NAME,
curses_destructor);
if (c_api_object == NULL) {
PyMem_Free(PyCurses_API);

This comment has been minimized.

@vstinner

vstinner Jan 21, 2021
Member

Hum, there is no a refleak here. Does it work to call curses_destructor() here?

This comment has been minimized.

@shihai1991

shihai1991 Jan 22, 2021
Author Member

Oh, thanks, updated.
Weird. ./python Lib/test/regrtest.py -ucurses test_curses -vv -R 3:3 can not catch this refleak.

This comment has been minimized.

@shihai1991

shihai1991 Jan 22, 2021
Author Member

Does it work to call curses_destructor() here?

No, it's can not work because capsule object haven't created

This comment has been minimized.

@vstinner

vstinner Jan 22, 2021
Member

Weird. ./python Lib/test/regrtest.py -ucurses test_curses -vv -R 3:3 can not catch this refleak.

test_curses doesn't test memory allocation failures. See _testcapi.set_nomemory() for that.

This comment has been minimized.

@vstinner

vstinner Jan 22, 2021
Member

Ah, and _curses doesn't use multiphase init API yet.

This comment has been minimized.

@shihai1991

shihai1991 Jan 23, 2021
Author Member

Got it.

@vstinner vstinner merged commit 2f12a1b into python:master Jan 22, 2021
10 checks passed
10 checks passed
Check for source changes
Details
Check if generated files are up to date
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20210122.10 succeeded
Details
Travis CI - Pull Request Build Passed
Details
bedevere/issue-number Issue number 41798 found
Details
bedevere/news "skip news" label found
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants