gh-86493: Modernize modules initialization code by serhiy-storchaka · Pull Request #106858 · python/cpython
Conversation
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().
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.
| if (o == NULL) { | ||
| Tkinter_TclError = PyErr_NewException("_tkinter.TclError", NULL, NULL); | ||
| if (PyModule_AddObjectRef(m, "TclError", Tkinter_TclError)) { | ||
| Py_DECREF(m); |
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.
Comment on lines -1505 to -1509
| if (PyModule_AddObject(m, name, o) == 0) { | ||
| return 0; | ||
| } | ||
| Py_DECREF(o); | ||
| return -1; |
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.
| return -1; | ||
| } | ||
| res = PyModule_AddObject( | ||
| res = PyModule_AddObjectRef( |
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.
| Py_INCREF(ErrorObject); | ||
| if (PyModule_AddObject(m, "error", ErrorObject) < 0) { | ||
| Py_DECREF(ErrorObject); | ||
| if (PyModule_AddObjectRef(m, "error", ErrorObject) < 0) { |
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 about PyModule_Add, but not about PyModule_AddObjectRef. There is a bug in PyModule_AddObjectRef declaration.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm too
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.
That's a big change, thanks.
jtcave pushed a commit to jtcave/cpython that referenced this pull request
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().
This was referenced