bpo-26868: Fix example usage of PyModule_AddObject. by brandtbucher · Pull Request #15725 · python/cpython

brandtbucher · 2019-09-07T15:52:10Z

Unlike other APIs that steal references, PyModule_AddObject only does it on success. However, most calls throughout the codebase don't manually Py_DECREF on failure as required by the implementation, which leads contributors to believe that it is not necessary. This PR fixes example usage and adds a note on this behavior in the documentation only.

There are over 200 bad calls throughout the codebase that are not addressed by this patch. Because there are so many bugs, fixing them all will likely require more care and likely several PRs (or a possible deprecation period and behavior change... it appears incorrect usage is actually more common than correct usage). I'm willing to do the work, but some direction on this would be nice.

https://bugs.python.org/issue26868

brandtbucher · 2019-09-07T15:57:07Z

@serhiy-storchaka

Doc/c-api/module.rst

Doc/extending/extending.rst

Doc/includes/sublist.c

brandtbucher · 2019-09-09T17:55:23Z

Thanks for the feedback @serhiy-storchaka! Changes applied, tests are passing.

serhiy-storchaka · 2019-09-10T08:37:14Z

Doc/extending/extending.rst

-       Py_INCREF(SpamError);
-       PyModule_AddObject(m, "error", SpamError);
+       Py_XINCREF(SpamError);
+       if (PyModule_AddObject(m, "error", SpamError)) {


I suggest to add also < 0 in all other checks for PyModule_AddObject() result.

Is this an agreed-upon style? I had always thought it was a matter of taste.

In either case, I guess all of these files should at least be consistent...

Add the extra test with < 0

ghost · 2019-09-11T03:53:17Z

There are three sytles currently:

if (PyModule_AddObject())
if (PyModule_AddObject() < 0)
if (PyModule_AddObject() == -1)

After some thinking, I think the second one is not bad.

Usually APIs use a negative return number to indicate failure.
Even the reader is not familiar with Python code, if he/she sees < 0, may guess that it's likely to detect failure.
So style 2 is a bit better than style 1.

The third style may produce a very slightly longer machine code:

; 19   :     if (PyModule_AddObject())
  00004	e8 00 00 00 00	 call	 ?PyModule_AddObject@@YAHXZ ; PyModule_AddObject
  00009	85 c0		 test	 eax, eax
  0000b	74 07		 je	 SHORT $LN2@main

; 21   :     if (PyModule_AddObject() < 0)
  00004	e8 00 00 00 00	 call	 ?PyModule_AddObject@@YAHXZ ; PyModule_AddObject
  00009	85 c0		 test	 eax, eax
  0000b	7d 07		 jge	 SHORT $LN2@main

; 23   :     if (PyModule_AddObject() == -1)
  00004	e8 00 00 00 00	 call	 ?PyModule_AddObject@@YAHXZ ; PyModule_AddObject
  00009	83 f8 ff	 cmp	 eax, -1
  0000c	75 07		 jne	 SHORT $LN2@main

And < 0 is likely to indicate that all failures are covered, not a specific failure.
So style 2 is a bit better than style 3.

There are reasons, although the reasons are weak. 😂

matrixise

Hi, Thank you for your contribution but could you add the small requested changes.

Thank you

matrixise · 2019-09-12T08:16:27Z

Doc/extending/extending.rst


-       if (c_api_object != NULL)
-           PyModule_AddObject(m, "_C_API", c_api_object);
+       if (PyModule_AddObject(m, "_C_API", c_api_object)) {


ditto, change the example with if (PyModule_AddObject(m, "_C_API", c_api_object) < 0)

matrixise · 2019-09-12T08:16:44Z

Doc/extending/newtypes_tutorial.rst


-   PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+   Py_INCREF(&CustomType);
+   if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:16:53Z

Doc/extending/newtypes_tutorial.rst


       Py_INCREF(&SubListType);
-       PyModule_AddObject(m, "SubList", (PyObject *) &SubListType);
+       if (PyModule_AddObject(m, "SubList", (PyObject *) &SubListType)) {


matrixise · 2019-09-12T08:16:59Z

Doc/includes/custom.c


    Py_INCREF(&CustomType);
-    PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:17:06Z

Doc/includes/custom2.c


    Py_INCREF(&CustomType);
-    PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:17:13Z

Doc/includes/custom3.c


    Py_INCREF(&CustomType);
-    PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:17:19Z

Doc/includes/custom4.c


    Py_INCREF(&CustomType);
-    PyModule_AddObject(m, "Custom", (PyObject *) &CustomType);
+    if (PyModule_AddObject(m, "Custom", (PyObject *) &CustomType)) {


matrixise · 2019-09-12T08:17:26Z

Doc/includes/sublist.c


    Py_INCREF(&SubListType);
-    PyModule_AddObject(m, "SubList", (PyObject *) &SubListType);
+    if (PyModule_AddObject(m, "SubList", (PyObject *) &SubListType)) {


matrixise

Thank you for your contribution, I have fixed the remarks with < 0 in the examples. I am going to merge it.

bedevere-bot · 2019-09-12T12:11:23Z

@matrixise: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2019-09-12T12:11:23Z

Thanks @brandtbucher for the PR, and @matrixise for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

* Add a note to the PyModule_AddObject docs. * Correct example usages of PyModule_AddObject. * Whitespace. * Clean up wording. * 📜🤖 Added by blurb_it. * First code review. * Add < 0 in the tests with PyModule_AddObject (cherry picked from commit 224b8aa) Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

bedevere-bot · 2019-09-12T12:11:33Z

GH-16037 is a backport of this pull request to the 3.8 branch.

* Add a note to the PyModule_AddObject docs. * Correct example usages of PyModule_AddObject. * Whitespace. * Clean up wording. * 📜🤖 Added by blurb_it. * First code review. * Add < 0 in the tests with PyModule_AddObject (cherry picked from commit 224b8aa) Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

bedevere-bot · 2019-09-12T12:11:40Z

GH-16038 is a backport of this pull request to the 3.7 branch.

* Add a note to the PyModule_AddObject docs. * Correct example usages of PyModule_AddObject. * Whitespace. * Clean up wording. * 📜🤖 Added by blurb_it. * First code review. * Add < 0 in the tests with PyModule_AddObject (cherry picked from commit 224b8aa) Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>

brandtbucher · 2019-09-12T13:25:56Z

Thanks @matrixise!

brandtbucher added 4 commits September 7, 2019 07:53

Add a note to the PyModule_AddObject docs.

90ae263

Correct example usages of PyModule_AddObject.

4b08887

Whitespace.

aef02b8

Clean up wording.

474a266

the-knights-who-say-ni added the CLA signed label Sep 7, 2019

bedevere-bot added the awaiting review label Sep 7, 2019

brandtbucher added needs backport to 3.7 docs Documentation in the Doc dir labels Sep 7, 2019

📜🤖 Added by blurb_it.

a005823

serhiy-storchaka reviewed Sep 7, 2019

View reviewed changes

Yhg1s added awaiting changes and removed awaiting review labels Sep 9, 2019

brandtbucher mentioned this pull request Sep 9, 2019

bpo-38037: Fix reference counters in signal module #15701

Closed

First code review.

bb345d9

serhiy-storchaka reviewed Sep 10, 2019

View reviewed changes

matrixise requested changes Sep 12, 2019

View reviewed changes

matrixise self-assigned this Sep 12, 2019

Add < 0 in the tests with PyModule_AddObject

37b6b37

matrixise approved these changes Sep 12, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 12, 2019

matrixise approved these changes Sep 12, 2019

View reviewed changes

matrixise merged commit 224b8aa into python:master Sep 12, 2019

bedevere-bot removed the awaiting merge label Sep 12, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 12, 2019

bedevere-bot removed the needs backport to 3.7 label Sep 12, 2019

brandtbucher deleted the safe-add-object branch September 12, 2019 13:26

berkerpeksag mentioned this pull request Oct 29, 2022

Document PyModule_AddObject's behavior on error #71055

Closed

Uh oh!

Conversation

brandtbucher commented Sep 7, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandtbucher commented Sep 7, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brandtbucher commented Sep 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 11, 2019

Uh oh!

matrixise left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matrixise left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

miss-islington commented Sep 12, 2019

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

bedevere-bot commented Sep 12, 2019

Uh oh!

brandtbucher commented Sep 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

brandtbucher commented Sep 7, 2019 •

edited by bedevere-bot

Loading