-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
bpo-41798: Allocate the _curses._C_API on the heap memory. #24186
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
Conversation
@@ -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 *)); |
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.
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 ;-)
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.
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 :)
c_api_object = PyCapsule_New(PyCurses_API, PyCurses_CAPSULE_NAME, | ||
curses_destructor); | ||
if (c_api_object == NULL) { | ||
PyMem_Free(PyCurses_API); |
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.
Hum, there is no a refleak here. Does it work to call curses_destructor() here?
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.
Oh, thanks, updated.
Weird. ./python Lib/test/regrtest.py -ucurses test_curses -vv -R 3:3
can not catch this refleak.
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.
Does it work to call curses_destructor() here?
No, it's can not work because capsule object haven't created
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.
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.
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.
Ah, and _curses doesn't use multiphase init API yet.
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.
Got it.
https://bugs.python.org/issue41798