gh-86493: Modernize modules initialization code#106858

serhiy-storchaka · 2023-07-18T10:06:48Z

Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().

PyModule_AddObject() is only left in two modules: _testcapi and _testbuffer. But that code ignores any failures for now. They need more significant rewriting.

Issue: C API: Add PyModule_Add() function #86493

📚 Documentation preview 📚: https://cpython-previews--106858.org.readthedocs.build/

Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().

vstinner

LGTM, but a few compiler warnings should be fixed first: see GHA job results, especially the Ubuntu job which logged many warnings.

Doc/extending/newtypes_tutorial.rst

Modules/_tkinter.c

vstinner · 2023-07-18T10:32:33Z

Modules/_tkinter.c

- if (o == NULL){
+Tkinter_TclError = PyErr_NewException("_tkinter.TclError", NULL, NULL);
+ if (PyModule_AddObjectRef(m, "TclError", Tkinter_TclError)){
 Py_DECREF(m);


Note for later: i prefer "exec" functions which returns -1 on error, and the caller is responsible to manage the module refcount.

Modules/binascii.c

Modules/xxlimited_35.c

Modules/zlibmodule.c

Doc/extending/newtypes_tutorial.rst

serhiy-storchaka · 2023-07-18T11:11:43Z

Modules/_lzmamodule.c

-if (PyModule_AddObject(m, name, o) ==0){
-return0;
- }
-Py_DECREF(o);
-return-1;


It is very confusing code with inverse logic. I thought there was a bug.

serhiy-storchaka · 2023-07-18T11:18:25Z

Modules/_testcapi/structmember.c

 return-1;
 }
-res=PyModule_AddObject(
+res=PyModule_AddObjectRef(


I think there was a bug.

serhiy-storchaka · 2023-07-18T11:29:51Z

Modules/xxlimited_35.c

-Py_INCREF(ErrorObject);
-if (PyModule_AddObject(m, "error", ErrorObject) <0){
-Py_DECREF(ErrorObject);
+if (PyModule_AddObjectRef(m, "error", ErrorObject) <0){


It is interesting, that the compiler complains about PyModule_Add, but not about PyModule_AddObjectRef. There is a bug in PyModule_AddObjectRef declaration.

corona10

lgtm too

vstinner

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.

Doc/extending/newtypes_tutorial.rst

vstinner · 2023-07-25T12:55:21Z

That's a big change, thanks.

Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().

pythongh-86493: Modernize modules initialization code
7bee7f7
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().

serhiy-storchaka requested a review from vstinner July 18, 2023 10:06

serhiy-storchaka requested review from abalkin, corona10, pganssle, rhettinger and tiran as code owners July 18, 2023 10:06

bedevere-bot added the awaiting core review label Jul 18, 2023

bedevere-bot mentioned this pull request Jul 18, 2023
C API: Add PyModule_Add() function #86493
Closed

vstinner approved these changes Jul 18, 2023
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jul 18, 2023

Fix compiler errors and warnings. Polish the code.
cdd8908

serhiy-storchaka commented Jul 18, 2023
View reviewed changes

corona10 approved these changes Jul 18, 2023
View reviewed changes

serhiy-storchaka added the skip news label Jul 18, 2023

vstinner approved these changes Jul 18, 2023
View reviewed changes

Doc/extending/newtypes_tutorial.rstShow resolvedHide resolved

rhettinger removed their request for review July 20, 2023 13:49

Merge branch 'main' into use-PyModule_Add
7c30d2d

serhiy-storchaka merged commit 329e4a1 into python:mainJul 25, 2023

bedevere-bot removed the awaiting merge label Jul 25, 2023

serhiy-storchaka deleted the use-PyModule_Add branch July 25, 2023 11:34

jtcave pushed a commit to jtcave/cpython that referenced this pull request Jul 27, 2023
pythongh-86493: Modernize modules initialization code (pythonGH-106858)
dc61706
Use PyModule_Add() or PyModule_AddObjectRef() instead of soft deprecated PyModule_AddObject().

This was referenced Feb 8, 2024
gh-83004: Clean up refleaks in ssl, socket initialisation #99044
Closed
Improve stdlib module initialization error handling. #83004
Closed
gh-83004: Clean up refleaks in _decimal initialisation #99043
Closed

Uh oh!

gh-86493: Modernize modules initialization code#106858

gh-86493: Modernize modules initialization code #106858

Uh oh!

Conversation

serhiy-storchaka commented Jul 18, 2023• edited by github-actions botLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinnerJul 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchakaJul 18, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaJul 18, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaJul 18, 2023

Choose a reason for hiding this comment

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

serhiy-storchaka commented Jul 18, 2023•
edited by github-actions bot
Loading