[issue42327] Add PyModule_Add()

[Łukasz Langa]

Change by Łukasz Langa :


--
nosy: +lukasz.langa
nosy_count: 3.0 -> 4.0
pull_requests: +27089
pull_request: https://github.com/python/cpython/pull/28741

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[STINNER Victor]

STINNER Victor  added the comment:

Note for myself: I added PyModule_AddObjectRef() to 
https://github.com/pythoncapi/pythoncapi_compat If it's renamed, 
pythoncapi_compat must also be updated.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
pull_requests: +22335
pull_request: https://github.com/python/cpython/pull/23443

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[STINNER Victor]

STINNER Victor  added the comment:

> PyModule_AddObjectRef() is just Py_XINCREF() followed by PyModule_Add(). But 
> since most values added to the module are new references, Py_XINCREF() is 
> usually not needed.

There is no general rule. I saw two main cases.


(A) Create an object only to add it into the module. PyModule_Add() and 
PyModule_AddObject() are good for that case.

Example in the array module:

PyObject *typecodes = PyUnicode_DecodeASCII(buffer, p - buffer, NULL);
if (PyModule_AddObject(m, "typecodes", typecodes) < 0) {
Py_XDECREF(typecodes);
return -1;
}

This code can be rewritten with PyModule_Add():

PyObject *typecodes = PyUnicode_DecodeASCII(buffer, p - buffer, NULL);
if (PyModule_Add(m, "typecodes", typecodes) < 0) {
return -1;
}

Py_XDECREF(typecodes) is no longer needed using PyModule_Add().


(B) Add an existing object into the module, but the objet is already stored 
elsewhere. PyModule_AddObjectRef() is good for that case. It became common to 
have this case when an object is also stored in the module state. 

Example in _ast:

state->AST_type = PyType_FromSpec(_type_spec);
if (!state->AST_type) {
return 0;
}
(...)
if (PyModule_AddObjectRef(m, "AST", state->AST_type) < 0) {
return -1;
}

state->AST_type and module attribute both hold a strong reference to the type.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

PyModule_Add() allows to make such macro much simpler:

#define MOD_ADD(name, expr) \
do { \
if (PyModule_Add(mod, name, expr) < 0) { \
return -1; \
} \
} while (0)

PyModule_AddObjectRef() is just Py_XINCREF() followed by PyModule_Add(). But 
since most values added to the module are new references, Py_XINCREF() is 
usually not needed. The PyModule_Add* API is a convenient API. It is not 
necessary, you can use PyModule_GetDict() + PyDict_SetItemString(), but with 
this API it is easier. And PyModule_Add() is a correct PyModule_AddObject() 
(which was broken a long time ago and cannot be fixed now) and is more 
convenient than PyModule_AddObjectRef().

PyModule_AddIntConstant() and PyModule_AddStringConstant() can be easily 
expressed in terms of PyModule_Add():

PyModule_Add(m, name, PyLong_FromLong(value))
PyModule_Add(m, name, PyUnicode_FromString(value))

And it is easy to combine it with other functions: PyLong_FromLongLong(), 
PyLong_FromUnsignedLong(), PyLong_FromVoidPtr(), PyFloat_FromDouble(), 
PyCapsule_New(), PyType_FromSpec(), etc.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[STINNER Victor]

STINNER Victor  added the comment:

In practice, PyModule_AddRef(mod, obj) behaves as PyModule_Add(mod, 
Py_NewRef(obj)) if I understood correctly.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[STINNER Victor]

STINNER Victor  added the comment:

If PyModule_Add() is added, I would suggest to rename PyModule_AddObjectRef() 
to PyModule_AddRef() for consistency.

IMHO PyModule_AddObjectRef() remains useful even if PyModule_Add() is added.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[STINNER Victor]

STINNER Victor  added the comment:

I'm using more and more often such macro:

#define MOD_ADD(name, expr) \
do { \
PyObject *obj = (expr); \
if (obj == NULL) { \
return -1; \
} \
if (PyModule_AddObjectRef(mod, name, obj) < 0) { \
Py_DECREF(obj); \
return -1; \
} \
Py_DECREF(obj); \
} while (0)

Would PyModule_Add() replace such macro?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[STINNER Victor]

STINNER Victor  added the comment:

Oh, I just rejected PR 17298. Copy of my message:
---
I added PyModule_AddObjectRef() which uses strong references, rather than only 
stealing a reference on success.

I also enhanced the documentation to show concrete examples:
https://docs.python.org/dev/c-api/module.html#c.PyModule_AddObjectRef

I modified a few extension to use PyModule_AddObjectRef(). Sometimes, 
PyModule_AddObject() is more appropriate. Sometimes, PyModule_AddObjectRef() is 
more appropriate. Both functions are relevant, and I don't see a clear winner.

I agree than fixing existing code is painful, but I hope that new code using 
mostly PyModule_AddObjectRef() would be simpler to review. I'm not sure that 
it's simpler to write new code using PyModule_AddObjectRef(), since you might 
need more Py_DECREF() calls.

My intent is to have more "regular" code about reference counting. See also: 
https://bugs.python.org/issue42294

Since you wrote that this API is a band aid on a broken API, I consider that 
you are fine with rejecting it and move on to the new PyModule_AddObjectRef().

Anyway, thanks for you attempt to make the C API less broken :-)
---

I added PyModule_AddObjectRef() in bpo-163574.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[Brandt Bucher]

Brandt Bucher  added the comment:

See also:

https://bugs.python.org/issue38823

https://github.com/python/cpython/pull/17298

--
nosy: +brandtbucher

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
keywords: +patch
pull_requests: +22137
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/23240

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue42327] Add PyModule_Add()

[Serhiy Storchaka]

New submission from Serhiy Storchaka :

There is a design flaw in PyModule_AddObject(). It steals reference of its 
value only if the result is success. To avoid leaks it should be used in the 
following form:

PyObject *tmp = ;
if (PyModule_AddObject(name, name, tmp) < 0) {
Py_XDECREF(tmp);
goto error;
}

It is inconvenient and many code forgot to use a temporary variable and call 
Py_XDECREF().

It was not intention, but it is too late to change this behavior now, because 
some code calls Py_XDECREF() if PyModule_AddObject() fails. Fixing 
PyModule_AddObject() now will break hard such code.

There was an idea to make the change gradual, controlled by a special macro 
(see issue26871). But it still has significant risk.

I propose to add new function PyModule_Add() which always steals reference to 
its argument. It is more convenient and allows to get rid of temporary variable:

if (PyModule_Add(name, name, ) < 0) {
goto error;
}

I choose name PyModule_Add because it is short, and allow to write the call in 
one line with moderately long expression  (like 
PyFloat_FromDouble(...) or PyLong_FromUnsignedLong(...)).

--
components: C API
messages: 380794
nosy: serhiy.storchaka, vstinner
priority: normal
severity: normal
status: open
title: Add PyModule_Add()
type: enhancement
versions: Python 3.10

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com