Skip to content

bpo-42327: Add PyModule_Add() (smaller). #23443

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jul 18, 2023
65 changes: 31 additions & 34 deletions Doc/c-api/module.rst
Original file line number Diff line number Diff line change
Expand Up @@ -486,12 +486,29 @@ state:
.. versionadded:: 3.10


.. c:function:: int PyModule_Add(PyObject *module, const char *name, PyObject *value)

Similar to :c:func:`PyModule_AddObjectRef`, but "steals" a reference
to *value*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyModule_AddObjectRef() must not be called with NULL. Can you please elaborate the behavior when value is NULL? Something like:

If value is NULL, just return -1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyModule_AddObjectRef() can be called with NULL if an exception is set. Actually, some of my fixes which use PyModule_AddObjectRef() rely on this.

It can be called with a result of function that returns a new reference
without bothering to check its result or even saving it to a variable.

Example usage::

if (PyModule_Add(module, "spam", PyBytes_FromString(value)) < 0) {
goto error;
}

.. versionadded:: 3.13


.. c:function:: int PyModule_AddObject(PyObject *module, const char *name, PyObject *value)

Similar to :c:func:`PyModule_AddObjectRef`, but steals a reference to
*value* on success (if it returns ``0``).

The new :c:func:`PyModule_AddObjectRef` function is recommended, since it is
The new :c:func:`PyModule_Add` or :c:func:`PyModule_AddObjectRef`
functions are recommended, since it is
easy to introduce reference leaks by misusing the
:c:func:`PyModule_AddObject` function.

Expand All @@ -501,44 +518,24 @@ state:
only decrements the reference count of *value* **on success**.

This means that its return value must be checked, and calling code must
:c:func:`Py_DECREF` *value* manually on error.
:c:func:`Py_XDECREF` *value* manually on error.

Example usage::

static int
add_spam(PyObject *module, int value)
{
PyObject *obj = PyLong_FromLong(value);
if (obj == NULL) {
return -1;
}
if (PyModule_AddObject(module, "spam", obj) < 0) {
Py_DECREF(obj);
return -1;
}
// PyModule_AddObject() stole a reference to obj:
// Py_DECREF(obj) is not needed here
return 0;
}

The example can also be written without checking explicitly if *obj* is
``NULL``::
PyObject *obj = PyBytes_FromString(value);
if (PyModule_AddObject(module, "spam", obj) < 0) {
// If 'obj' is not NULL and PyModule_AddObject() failed,
// 'obj' strong reference must be deleted with Py_XDECREF().
// If 'obj' is NULL, Py_XDECREF() does nothing.
Py_XDECREF(obj);
goto error;
}
// PyModule_AddObject() stole a reference to obj:
// Py_XDECREF(obj) is not needed here.

static int
add_spam(PyObject *module, int value)
{
PyObject *obj = PyLong_FromLong(value);
if (PyModule_AddObject(module, "spam", obj) < 0) {
Py_XDECREF(obj);
return -1;
}
// PyModule_AddObject() stole a reference to obj:
// Py_DECREF(obj) is not needed here
return 0;
}
.. deprecated:: 3.13

Note that ``Py_XDECREF()`` should be used instead of ``Py_DECREF()`` in
this case, since *obj* can be ``NULL``.
:c:func:`PyModule_AddObject` is :term:`soft deprecated`.


.. c:function:: int PyModule_AddIntConstant(PyObject *module, const char *name, long value)
Expand Down
1 change: 1 addition & 0 deletions Doc/data/stable_abi.dat

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions Doc/whatsnew/3.13.rst
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,11 @@ New Features
If the assertion fails, make sure that the size is set before.
(Contributed by Victor Stinner in :gh:`106168`.)

* Add :c:func:`PyModule_Add` function: similar to
:c:func:`PyModule_AddObjectRef` and :c:func:`PyModule_AddObject` but
always steals a reference to the value.
(Contributed by Serhiy Storchaka in :gh:`86493`.)

Porting to Python 3.13
----------------------

Expand Down
12 changes: 9 additions & 3 deletions Include/modsupport.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,18 @@ PyAPI_FUNC(PyObject *) Py_BuildValue(const char *, ...);
PyAPI_FUNC(PyObject *) Py_VaBuildValue(const char *, va_list);

// Add an attribute with name 'name' and value 'obj' to the module 'mod.
// On success, return 0 on success.
// On success, return 0.
// On error, raise an exception and return -1.
PyAPI_FUNC(int) PyModule_AddObjectRef(PyObject *mod, const char *name, PyObject *value);

// Similar to PyModule_AddObjectRef() but steal a reference to 'obj'
// (Py_DECREF(obj)) on success (if it returns 0).
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x030d0000
// Similar to PyModule_AddObjectRef() but steal a reference to 'value'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to add "and value can be NULL".

PyAPI_FUNC(int) PyModule_Add(PyObject *mod, const char *name, PyObject *value);
#endif /* Py_LIMITED_API */

// Similar to PyModule_AddObjectRef() and PyModule_Add() but steal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not similar to PyModule_AddObjectRef(): value can be NULL.

Suggested change
// Similar to PyModule_AddObjectRef() and PyModule_Add() but steal
// Similar to PyModule_Add() but steal

// a reference to 'value' on success and only on success.
// Errorprone. Should not be used in new code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's important important to put it in the documentation. This kind of definition fits perfectly with the Soft Deprecated status. Maybe also soft deprecate this API, similar to getopt and optparse soft deprecation. Example with getopt:

.. deprecated:: 3.13
   The :mod:`getopt` module is :term:`soft deprecated` and will not be
   developed further; development will continue with the :mod:`argparse`
   module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest to convert the note block into deprecated?

PyAPI_FUNC(int) PyModule_AddObject(PyObject *mod, const char *, PyObject *value);

PyAPI_FUNC(int) PyModule_AddIntConstant(PyObject *, const char *, long);
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_stable_abi_ctypes.py

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add :func:`PyModule_Add` function: similar to :c:func:`PyModule_AddObjectRef` and :c:func:`PyModule_AddObject`, but always steals a reference to the value.
2 changes: 2 additions & 0 deletions Misc/stable_abi.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2444,3 +2444,5 @@
added = '3.13'
[function.PyMapping_GetOptionalItemString]
added = '3.13'
[function.PyModule_Add]
added = '3.13'
1 change: 1 addition & 0 deletions PC/python3dll.c

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 10 additions & 19 deletions Python/modsupport.c
Original file line number Diff line number Diff line change
Expand Up @@ -606,13 +606,16 @@ PyModule_AddObjectRef(PyObject *mod, const char *name, PyObject *value)
PyModule_GetName(mod));
return -1;
}

if (PyDict_SetItemString(dict, name, value)) {
return -1;
}
return 0;
return PyDict_SetItemString(dict, name, value);
}

int
PyModule_Add(PyObject *mod, const char *name, PyObject *value)
{
int res = PyModule_AddObjectRef(mod, name, value);
Py_XDECREF(value);
return res;
}

int
PyModule_AddObject(PyObject *mod, const char *name, PyObject *value)
Expand All @@ -627,25 +630,13 @@ PyModule_AddObject(PyObject *mod, const char *name, PyObject *value)
int
PyModule_AddIntConstant(PyObject *m, const char *name, long value)
{
PyObject *obj = PyLong_FromLong(value);
if (!obj) {
return -1;
}
int res = PyModule_AddObjectRef(m, name, obj);
Py_DECREF(obj);
return res;
return PyModule_Add(m, name, PyLong_FromLong(value));
}

int
PyModule_AddStringConstant(PyObject *m, const char *name, const char *value)
{
PyObject *obj = PyUnicode_FromString(value);
if (!obj) {
return -1;
}
int res = PyModule_AddObjectRef(m, name, obj);
Py_DECREF(obj);
return res;
return PyModule_Add(m, name, PyUnicode_FromString(value));
}

int
Expand Down