bpo-42522: Add Py_Borrow() and Py_XBorrow() functions#23591
bpo-42522: Add Py_Borrow() and Py_XBorrow() functions#23591vstinner wants to merge 3 commits intopython:masterfrom vstinner:py_borrow
Conversation
Added _Py_Borrow() and _Py_XBorrow() functions to borrow a reference to an object: decrement the object reference count and return the object.
Doc/c-api/refcounting.rst
Outdated
| :c:func:`Py_NewRef` function can be used to create a new :term:`strong | ||
| reference`. | ||
|
|
||
| In other terms, the :c:func:`_Py_Borrow` function should be avoided whenever |
There was a problem hiding this comment.
I'm wondering if the documentation should explain very briefly why borrowing references is bad? The documentation shouldn't go on a long rant, but at the moment the reader might be left wondering why _Py_Borrow should be avoided.
There was a problem hiding this comment.
The "term" syntax creates a link to: https://docs.python.org/dev/glossary.html#term-borrowed-reference which is where I explained in which cases borrowed references are unsafe.
I also wrote https://docs.python.org/dev/glossary.html#term-strong-reference which describes reference counting.
|
@hodgestar: Would you mind to review the updated PR? |
|
|
||
|
|
||
| static PyObject* | ||
| test_refcount(PyObject* self, PyObject* ignored) |
There was a problem hiding this comment.
nitpick: PyObject *self, PyObject *ignored
shihai1991
left a comment
shihai1991
left a comment
There was a problem hiding this comment.
This PR LGTM. I just to say pls take careful when using this function :)
| Py_INCREF(obj); | ||
| PyObject *xborrowed = _Py_XBorrow(obj); | ||
| assert(xborrowed == obj); | ||
| assert(Py_REFCNT(obj) == 1); |
There was a problem hiding this comment.
Should we perhaps also test that Py_XNewRef(NULL) and _Py_XBorrow(NULL) behave as expected?
hodgestar
left a comment
hodgestar
left a comment
There was a problem hiding this comment.
Left a small suggestion for strengthening the test, but +1 on these changes.
Looking forward to a borrowed-reference-free C API in 3.10! :D
|
https://bugs.python.org/issue42522#msg382270:
Should I rename _Py_Borrow() to _Py_StealRef()? |
I see why the commenter suggested The function itself doesn't so much steals a reference as throw it away (a typical stolen reference would only be DECREFed much later). The effect for the caller is that the reference is now borrowed and since the only intended use case for this function is to turn a reference into a borrowed reference, the current name seems good to me. |
5 participants
https://bugs.python.org/issue42522