diff --git a/Include/cpython/object.h b/Include/cpython/object.h index 0d0f1e23673e58..b018dabf9d862f 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -309,41 +309,37 @@ _PyObject_GenericSetAttrWithDict(PyObject *, PyObject *, PyAPI_FUNC(PyObject *) _PyObject_FunctionStr(PyObject *); -/* Safely decref `dst` and set `dst` to `src`. +/* Safely decref `op` and set `op` to `op2`. * * As in case of Py_CLEAR "the obvious" code can be deadly: * - * Py_DECREF(dst); - * dst = src; + * Py_DECREF(op); + * op = op2; * * The safe way is: * - * Py_SETREF(dst, src); + * Py_SETREF(op, op2); * - * That arranges to set `dst` to `src` _before_ decref'ing, so that any code - * triggered as a side-effect of `dst` getting torn down no longer believes - * `dst` points to a valid object. + * That arranges to set `op` to `op2` _before_ decref'ing, so that any code + * triggered as a side-effect of `op` getting torn down no longer believes + * `op` points to a valid object. * - * gh-98724: Use the _tmp_dst_ptr variable to evaluate the 'dst' macro argument - * exactly once, to prevent the duplication of side effects in this macro. + * Py_XSETREF is a variant of Py_SETREF that uses Py_XDECREF instead of + * Py_DECREF. */ -#define Py_SETREF(dst, src) \ - do { \ - PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \ - PyObject *_tmp_dst = (*_tmp_dst_ptr); \ - *_tmp_dst_ptr = _PyObject_CAST(src); \ - Py_DECREF(_tmp_dst); \ + +#define Py_SETREF(op, op2) \ + do { \ + PyObject *_py_tmp = _PyObject_CAST(op); \ + (op) = (op2); \ + Py_DECREF(_py_tmp); \ } while (0) -/* Py_XSETREF() is a variant of Py_SETREF() that uses Py_XDECREF() instead of - * Py_DECREF(). - */ -#define Py_XSETREF(dst, src) \ - do { \ - PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \ - PyObject *_tmp_dst = (*_tmp_dst_ptr); \ - *_tmp_dst_ptr = _PyObject_CAST(src); \ - Py_XDECREF(_tmp_dst); \ +#define Py_XSETREF(op, op2) \ + do { \ + PyObject *_py_tmp = _PyObject_CAST(op); \ + (op) = (op2); \ + Py_XDECREF(_py_tmp); \ } while (0) diff --git a/Include/object.h b/Include/object.h index 73bebcde993cbf..f2af428e2bb973 100644 --- a/Include/object.h +++ b/Include/object.h @@ -575,21 +575,16 @@ static inline void Py_DECREF(PyObject *op) * one of those can't cause problems -- but in part that relies on that * Python integers aren't currently weakly referencable. Best practice is * to use Py_CLEAR() even if you can't think of a reason for why you need to. - * - * gh-98724: Use the _py_tmp_ptr variable to evaluate the macro argument - * exactly once, to prevent the duplication of side effects in this macro. */ -#define Py_CLEAR(op) \ - do { \ - PyObject **_py_tmp_ptr = _Py_CAST(PyObject**, &(op)); \ - if (*_py_tmp_ptr != NULL) { \ - PyObject* _py_tmp = (*_py_tmp_ptr); \ - *_py_tmp_ptr = NULL; \ - Py_DECREF(_py_tmp); \ - } \ +#define Py_CLEAR(op) \ + do { \ + PyObject *_py_tmp = _PyObject_CAST(op); \ + if (_py_tmp != NULL) { \ + (op) = NULL; \ + Py_DECREF(_py_tmp); \ + } \ } while (0) - /* Function to use in case the object pointer can be NULL: */ static inline void Py_XINCREF(PyObject *op) { diff --git a/Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst b/Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst deleted file mode 100644 index 2613b17924014b..00000000000000 --- a/Misc/NEWS.d/next/C API/2022-11-04-16-13-35.gh-issue-98724.p0urWO.rst +++ /dev/null @@ -1,3 +0,0 @@ -The :c:macro:`Py_CLEAR`, :c:macro:`Py_SETREF` and :c:macro:`Py_XSETREF` macros -now only evaluate their argument once. If the argument has side effects, these -side effects are no longer duplicated. Patch by Victor Stinner. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 92a2a07e2a44c8..1291eff481cbb8 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -5684,91 +5684,6 @@ test_set_type_size(PyObject *self, PyObject *Py_UNUSED(ignored)) } -// Test Py_CLEAR() macro -static PyObject* -test_py_clear(PyObject *self, PyObject *Py_UNUSED(ignored)) -{ - // simple case with a variable - PyObject *obj = PyList_New(0); - if (obj == NULL) { - return NULL; - } - Py_CLEAR(obj); - assert(obj == NULL); - - // gh-98724: complex case, Py_CLEAR() argument has a side effect - PyObject* array[1]; - array[0] = PyList_New(0); - if (array[0] == NULL) { - return NULL; - } - - PyObject **p = array; - Py_CLEAR(*p++); - assert(array[0] == NULL); - assert(p == array + 1); - - Py_RETURN_NONE; -} - - -// Test Py_SETREF() and Py_XSETREF() macros, similar to test_py_clear() -static PyObject* -test_py_setref(PyObject *self, PyObject *Py_UNUSED(ignored)) -{ - // Py_SETREF() simple case with a variable - PyObject *obj = PyList_New(0); - if (obj == NULL) { - return NULL; - } - Py_SETREF(obj, NULL); - assert(obj == NULL); - - // Py_XSETREF() simple case with a variable - PyObject *obj2 = PyList_New(0); - if (obj2 == NULL) { - return NULL; - } - Py_XSETREF(obj2, NULL); - assert(obj2 == NULL); - // test Py_XSETREF() when the argument is NULL - Py_XSETREF(obj2, NULL); - assert(obj2 == NULL); - - // gh-98724: complex case, Py_SETREF() argument has a side effect - PyObject* array[1]; - array[0] = PyList_New(0); - if (array[0] == NULL) { - return NULL; - } - - PyObject **p = array; - Py_SETREF(*p++, NULL); - assert(array[0] == NULL); - assert(p == array + 1); - - // gh-98724: complex case, Py_XSETREF() argument has a side effect - PyObject* array2[1]; - array2[0] = PyList_New(0); - if (array2[0] == NULL) { - return NULL; - } - - PyObject **p2 = array2; - Py_XSETREF(*p2++, NULL); - assert(array2[0] == NULL); - assert(p2 == array2 + 1); - - // test Py_XSETREF() when the argument is NULL - p2 = array2; - Py_XSETREF(*p2++, NULL); - assert(array2[0] == NULL); - assert(p2 == array2 + 1); - - Py_RETURN_NONE; -} - - #define TEST_REFCOUNT() \ do { \ PyObject *obj = PyList_New(0); \ @@ -6560,8 +6475,6 @@ static PyMethodDef TestMethods[] = { {"pynumber_tobase", pynumber_tobase, METH_VARARGS}, {"without_gc", without_gc, METH_O}, {"test_set_type_size", test_set_type_size, METH_NOARGS}, - {"test_py_clear", test_py_clear, METH_NOARGS}, - {"test_py_setref", test_py_setref, METH_NOARGS}, {"test_refcount_macros", test_refcount_macros, METH_NOARGS}, {"test_refcount_funcs", test_refcount_funcs, METH_NOARGS}, {"test_py_is_macros", test_py_is_macros, METH_NOARGS},