GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-86493: Modernize modules initialization code by serhiy-storchaka · Pull Request #106858 · python/cpython

Conversation

serhiy-storchaka commented Jul 18, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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

vstinner left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/extending/newtypes_tutorial.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_tkinter.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_tkinter.c

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

vstinner Jul 18, 2023

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/binascii.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/xxlimited_35.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/zlibmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/zlibmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

Comment thread

Doc/extending/newtypes_tutorial.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_lzmamodule.c

Comment on lines -1505 to -1509

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

serhiy-storchaka Jul 18, 2023

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_testcapi/structmember.c

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

serhiy-storchaka Jul 18, 2023

Copy link

Copy Markdown

Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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) {

serhiy-storchaka Jul 18, 2023

Copy link

Copy Markdown

Member Author

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 about PyModule_Add, but not about PyModule_AddObjectRef. There is a bug in PyModule_AddObjectRef declaration.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

corona10 approved these changes

Jul 18, 2023

View reviewed changes

corona10 left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka added the skip news label

Jul 18, 2023

vstinner approved these changes

Jul 18, 2023

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/extending/newtypes_tutorial.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

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:main

Jul 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

vstinner commented Jul 25, 2023

Copy link

Copy Markdown

Member

That's a big change, thanks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

Reviewers

vstinner

vstinner approved these changes

corona10

corona10 approved these changes

pganssle

Awaiting requested review from pganssle

pganssle is a code owner

abalkin

Awaiting requested review from abalkin

tiran

Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

serhiy-storchaka commented Jul 18, 2023 • edited by github-actions Bot Loading 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!

vstinner Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jul 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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

serhiy-storchaka commented Jul 18, 2023 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.