Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-86493: Modernize modules initialization code#106858
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecatedPyModule_AddObject().
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.
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.
| o = PyErr_NewException("_tkinter.TclError", NULL, NULL); | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is very confusing code with inverse logic. I thought there was a bug.
Modules/_testcapi/structmember.c Outdated
| @@ -193,7 +193,7 @@ _PyTestCapi_Init_Structmember(PyObject *m) | |||
| if (res < 0) { | |||
| return -1; | |||
| } | |||
| res =PyModule_AddObject( | |||
| res =PyModule_AddObjectRef( | |||
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.
I think there was a bug.
Modules/xxlimited_35.c Outdated
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is interesting, that the compiler complains aboutPyModule_Add, but not aboutPyModule_AddObjectRef. There is a bug inPyModule_AddObjectRef declaration.
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.
lgtm too
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.
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.
That's a big change, thanks. |
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecatedPyModule_AddObject().
Uh oh!
There was an error while loading.Please reload this page.
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/