Uh oh!
There was an error while loading. Please reload this page.
gh-86493: Modernize modules initialization code - #106858
Conversation
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().
vstinner
left a comment
There was a problem hiding this comment.
LGTM, but a few compiler warnings should be fixed first: see GHA job results, especially the Ubuntu job which logged many warnings.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| if (o == NULL) { | ||
| Tkinter_TclError = PyErr_NewException("_tkinter.TclError", NULL, NULL); | ||
| if (PyModule_AddObjectRef(m, "TclError", Tkinter_TclError)) { | ||
| Py_DECREF(m); |
There was a problem hiding this comment.
Note for later: i prefer "exec" functions which returns -1 on error, and the caller is responsible to manage the module refcount.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| if (PyModule_AddObject(m, name, o) == 0) { | ||
| return 0; | ||
| } | ||
| Py_DECREF(o); | ||
| return -1; |
There was a problem hiding this comment.
It is very confusing code with inverse logic. I thought there was a bug.
| return -1; | ||
| } | ||
| res = PyModule_AddObject( | ||
| res = PyModule_AddObjectRef( |
There was a problem hiding this comment.
I think there was a bug.
| Py_INCREF(ErrorObject); | ||
| if (PyModule_AddObject(m, "error", ErrorObject) < 0) { | ||
| Py_DECREF(ErrorObject); | ||
| if (PyModule_AddObjectRef(m, "error", ErrorObject) < 0) { |
There was a problem hiding this comment.
It is interesting, that the compiler complains about PyModule_Add, but not about PyModule_AddObjectRef. There is a bug in PyModule_AddObjectRef declaration.
vstinner
left a comment
There was a problem hiding this comment.
LGTM. I didn't check individual refcount, I rely on Refleaks buildbots for that and other reviewers 😁 Overall the change LGTM and makes the code shorter and more regular. The old code was really hard to understand with INCREF/DECREF dance.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Jul 25, 2023
That's a big change, thanks. |
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().
PyModule_AddObject() is only left in two modules:
_testcapiand_testbuffer. But that code ignores any failures for now. They need more significant rewriting.📚 Documentation preview 📚: https://cpython-previews--106858.org.readthedocs.build/