8000 MNT: replace _PyDict_GetItemStringWithError with PyDict_GetItemStringRef by ngoldbaum · Pull Request #26282 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MNT: replace _PyDict_GetItemStringWithError with PyDict_GetItemStringRef #26282

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.

Sign up for GitHub

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 5 commits into from
Apr 17, 2024
+82 −62
Merged

MNT: replace _PyDict_GetItemStringWithError with PyDict_GetItemStringRef #26282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
407689c
MNT: replace _PyDict_GetItemStringWithError with PyDict_GetItemStringRef
ngoldbaum Apr 15, 2024
64ffcd1
MNT: fix issue spotted in code review
ngoldbaum Apr 15, 2024
5d67699
MNT: fix two more issues spotted in code review
ngoldbaum Apr 16, 2024
90c8049
MNT: more missing decrefs spotted in code review
ngoldbaum Apr 16, 2024
26f7421
MNT: delete unnecessary decrefs (covered by goto fail logic)
ngoldbaum Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments. 8000
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions numpy/_core/src/common/ufunc_override.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define _MULTIARRAYMODULE

#include "numpy/ndarraytypes.h"
#include "npy_pycompat.h"
#include "get_attr_string.h"
#include "npy_import.h"
#include "ufunc_override.h"
Expand Down Expand Up @@ -99,12 +100,11 @@ PyUFuncOverride_GetOutObjects(PyObject *kwds, PyObject **out_kwd_obj, PyObject *
*out_kwd_obj = NULL;
return -1;
}
/* borrowed reference */
*out_kwd_obj = _PyDict_GetItemStringWithError(kwds, "out");
if (*out_kwd_obj == NULL) {
if (PyErr_Occurred()) {
return -1;
}
int result = PyDict_GetItemStringRef(kwds, "out", out_kwd_obj);
if (result == -1) {
return -1;
}
else if (result == 0) {
Py_INCREF(Py_None);
*out_kwd_obj = Py_None;
return 0;
Expand All @@ -118,15 +118,14 @@ PyUFuncOverride_GetOutObjects(PyObject *kwds, PyObject **out_kwd_obj, PyObject *
seq = PySequence_Fast(*out_kwd_obj,
"Could not convert object to sequence");
if (seq == NULL) {
*out_kwd_obj = NULL;
Py_CLEAR(*out_kwd_obj);
return -1;
}
*out_objs = PySequence_Fast_ITEMS(seq);
*out_kwd_obj = seq;
Py_SETREF(*out_kwd_obj, seq);
return PySequence_Fast_GET_SIZE(seq);
}
else {
Py_INCREF(*out_kwd_obj);
*out_objs = out_kwd_obj;
return 1;
}
Expand Down
67 changes: 39 additions & 28 deletions numpy/_core/src/multiarray/ctors.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#include "numpy/npy_math.h"

#include "npy_config.h"

#include "npy_pycompat.h"
#include "npy_ctypes.h"

#include "multiarraymodule.h"
Expand Down Expand Up @@ -2180,10 +2180,10 @@ PyArray_FromInterface(PyObject *origin)
}

/* Get type string from interface specification */
attr = _PyDict_GetItemStringWithError(iface, "typestr");
if (attr == NULL) {
int result = PyDict_GetItemStringRef(iface, "typestr", &attr);
if (result <= 0) {
Py_DECREF(iface);
if (!PyErr_Occurred()) {
if (result == 0) {
PyErr_SetString(PyExc_ValueError,
"Missing __array_interface__ typestr");
}
Expand All @@ -2207,43 +2207,47 @@ PyArray_FromInterface(PyObject *origin)
* the 'descr' attribute.
*/
if (dtype->type_num == NPY_VOID) {
PyObject *descr = _PyDict_GetItemStringWithError(iface, "descr");
if (descr == NULL && PyErr_Occurred()) {
PyObject *descr = NULL;
result = PyDict_GetItemStringRef(iface, "descr", &descr);
if (result == -1) {
goto fail;
}
PyArray_Descr *new_dtype = NULL;
if (descr != NULL) {
if (result == 1) {
int is_default = _is_default_descr(descr, attr);
if (is_default < 0) {
Py_DECREF(descr);
goto fail;
}
if (!is_default) {
if (PyArray_DescrConverter2(descr, &new_dtype) != NPY_SUCCEED) {
Py_DECREF(descr);
goto fail;
Copy link
Member

Choose a reason for hiding this comment

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

Weird how we use goto fail here, but not below it. But OK.

}
if (new_dtype != NULL) {
Py_DECREF(dtype);
dtype = new_dtype;
Py_SETREF(dtype, new_dtype);
}
}

}

Py_DECREF(descr);
Copy link
Member

Choose a reason for hiding this comment

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

@ngoldbaum this is at the wrong indentation level. It's too late here, and I am not sure where to put a test though. cunumeric notices this, it seems they use void (I suspect unstructured but dunno), without a descr.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, thanks for catching. I unfortunately can't help with a test, I was doing a mechanical refactor here.

Copy link
Member

Choose a reason for hiding this comment

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

We must have some __array_interface__ tests. It shouldn't be hard to create a dict that runs into this, tbh. Although I could overlook it, it would be nice to just have.

Copy link
Member Author
@ngoldbaum ngoldbaum Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

My initial reading of your message was that it was a head's up and an ask for help. I'm happy to fix this but if, in the future, you want me to look at something, it would help me if you could say that explicitly in your initial message to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the indentation level is wrong, doesn't it just need to be an xdecref?

I also don't immediately see how to write a test for this, the existing __array_interface__ tests mostly use the ndarray __array_interface__ implementation. It would be a good idea to improve the tests there but I don't want to do that just to fix this bug.

Copy link
Member

Choose a reason for hiding this comment

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

Should have just created an issue. I wanted to look at it quickly, but realized that adding a test was not quite as quick as I had hoped, and hoped you might have a look.

}
Py_CLEAR(attr);

/* Get shape tuple from interface specification */
attr = _PyDict_GetItemStringWithError(iface, "shape");
if (attr == NULL) {
if (PyErr_Occurred()) {
return NULL;
}
result = PyDict_GetItemStringRef(iface, "shape", &attr);
if (result < 0) {
return NULL;
}
if (result == 0) {
/* Shape must be specified when 'data' is specified */
PyObject *data = _PyDict_GetItemStringWithError(iface, "data");
if (data == NULL && PyErr_Occurred()) {
int result = PyDict_ContainsString(iface, "data");
if (result < 0) {
Py_DECREF(attr);
return NULL;
}
else if (data != NULL) {
else if (result == 1) {
Py_DECREF(iface);
Py_DECREF(attr);
PyErr_SetString(PyExc_ValueError,
"Missing __array_interface__ shape");
return NULL;
Expand Down Expand Up @@ -2271,10 +2275,11 @@ PyArray_FromInterface(PyObject *origin)
}
}
}
Py_CLEAR(attr);

/* Get data buffer from interface specification */
attr = _PyDict_GetItemStringWithError(iface, "data");
if (attr == NULL && PyErr_Occurred()){
result = PyDict_GetItemStringRef(iface, "data", &attr);
if (result == -1){
return NULL;
}

Expand Down Expand Up @@ -2337,20 +2342,24 @@ PyArray_FromInterface(PyObject *origin)
PyBuffer_Release(&view);

/* Get offset number from interface specification */
attr = _PyDict_GetItemStringWithError(iface, "offset");
if (attr == NULL && PyErr_Occurred()) {
PyObject *offset = NULL;
result = PyDict_GetItemStringRef(iface, "offset", &offset);
if (result == -1) {
goto fail;
}
else if (attr) {
npy_longlong num = PyLong_AsLongLong(attr);
else if (result == 1) {
npy_longlong num = PyLong_AsLongLong(offset);
if (error_converting(num)) {
PyErr_SetString(PyExc_TypeError,
"__array_interface__ offset must be an integer");
Py_DECREF(offset);
goto fail;
}
data += num;
Py_DECREF(offset);
}
}
Py_CLEAR(attr);

ret = (PyArrayObject *)PyArray_NewFromDescrAndBase(
&PyArray_Type, dtype,
Expand All @@ -2376,11 +2385,11 @@ PyArray_FromInterface(PyObject *origin)
goto fail;
}
}
attr = _PyDict_GetItemStringWithError(iface, "strides");
if (attr == NULL && PyErr_Occurred()){
result = PyDict_GetItemStringRef(iface, "strides", &attr);
if (result == -1){
return NULL;
}
if (attr != NULL && attr != Py_None) {
if (result == 1 && attr != Py_None) {
if (!PyTuple_Check(attr)) {
PyErr_SetString(PyExc_TypeError,
"strides must be a tuple");
Expand All @@ -2404,12 +2413,14 @@ PyArray_FromInterface(PyObject *origin)
if (n) {
memcpy(PyArray_STRIDES(ret), strides, n*sizeof(npy_intp));
}
Py_DECREF(attr);
}
PyArray_UpdateFlags(ret, NPY_ARRAY_UPDATE_ALL);
Py_DECREF(iface);
return (PyObject *)ret;

fail:
Py_XDECREF(attr);
Py_XDECREF(dtype);
Py_XDECREF(iface);
return NULL;
Expand Down
17 changes: 9 additions & 8 deletions numpy/_core/src/multiarray/number.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include "numpy/arrayobject.h"

#include "npy_config.h"

#include "npy_pycompat.h"
#include "npy_import.h"
#include "common.h"
#include "number.h"
Expand Down Expand Up @@ -60,24 +60,25 @@ array_inplace_matrix_multiply(PyArrayObject *m1, PyObject *m2);
* Those not present will not be changed
*/

/* FIXME - macro contains a return */
#define SET(op) temp = _PyDict_GetItemStringWithError(dict, #op); \
if (temp == NULL && PyErr_Occurred()) { \
/* FIXME - macro contains returns */
#define SET(op) \
res = PyDict_GetItemStringRef(dict, #op, &temp); \
if (res == -1) { \
return -1; \
} \
else if (temp != NULL) { \
else if (res == 1) { \
if (!(PyCallable_Check(temp))) { \
Py_DECREF(temp); \
return -1; \
} \
Py_INCREF(temp); \
Py_XDECREF(n_ops.op); \
n_ops.op = temp; \
Py_XSETREF(n_ops.op, temp); \
}

NPY_NO_EXPORT int
_PyArray_SetNumericOps(PyObject *dict)
{
PyObject *temp = NULL;
int res;
SET(add);
SET(subtract);
SET(multiply);
Expand Down
15 changes: 7 additions & 8 deletions numpy/_core/src/umath/override.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#include "numpy/ndarraytypes.h"
#include "numpy/ufuncobject.h"
#include "npy_import.h"

#include "npy_pycompat.h"
#include "override.h"
#include "ufunc_override.h"

Expand Down Expand Up @@ -148,18 +148,17 @@ static int
normalize_signature_keyword(PyObject *normal_kwds)
{
/* If the keywords include `sig` rename to `signature`. */
PyObject* obj = _PyDict_GetItemStringWithError(normal_kwds, "sig");
if (obj == NULL && PyErr_Occurred()) {
PyObject* obj = NULL;
int result = PyDict_GetItemStringRef(normal_kwds, "sig", &obj);
if (result == -1) {
return -1;
}
if (obj != NULL) {
/*
* No INCREF or DECREF needed: got a borrowed reference above,
* and, unlike e.g. PyList_SetItem, PyDict_SetItem INCREF's it.
*/
if (result == 1) {
if (PyDict_SetItemString(normal_kwds, "signature", obj) < 0) {
Py_DECREF(obj);
return -1;
}
Py_DECREF(obj);
if (PyDict_DelItemString(normal_kwds, "sig") < 0) {
return -1;
}
Expand Down
9 changes: 6 additions & 3 deletions numpy/_core/src/umath/ufunc_object.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
#include <stddef.h>

#include "npy_config.h"

#include "npy_pycompat.h"
#include "npy_argparse.h"

#include "numpy/arrayobject.h"
Expand Down Expand Up @@ -142,8 +142,10 @@ PyUFunc_clearfperr()
NPY_NO_EXPORT int
set_matmul_flags(PyObject *d)
{
PyObject *matmul = _PyDict_GetItemStringWithError(d, "matmul");
if (matmul == NULL) {
PyObject *matmul = NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Does this need an init? If it does, might make sense to mention that in the Python docs, @vstinner, even if not a big deal. (I.e. that it effectively uses Py_SETREF(res, ...) internally.

But this would be a silly nitpick here to actually remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can check the doc: https://docs.python.org/dev/c-api/dict.html#c.PyDict_GetItemRef

Copy link
Member
@seberg seberg Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, the docs are fine. It isn't like result can be reasonably set to a e.g. a default value before the call: The code here unnecessarily initializes it, which made me wonder.

EDIT: Sorry, right... it lists that result is set on all paths, so it is very obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to initialize variables in C code even if I don't strictly need to. Probably a knee-jerk response from experiences of reading from an uninitialized variable causing heisenbugs.

int result = PyDict_GetItemStringRef(d, "matmul", &matmul);
if (result <= 0) {
// caller sets an error if one isn't already set
return -1;
}
/*
Expand All @@ -162,6 +164,7 @@ set_matmul_flags(PyObject *d)
NPY_ITER_UPDATEIFCOPY |
NPY_UFUNC_DEFAULT_OUTPUT_FLAGS) &
~NPY_ITER_OVERLAP_ASSUME_ELEMENTWISE;
Py_DECREF(matmul);
return 0;
}

Expand Down
19 changes: 13 additions & 6 deletions numpy/_core/src/umath/umathmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "numpy/arrayobject.h"
#include "numpy/ufuncobject.h"
#include "numpy/npy_3kcompat.h"
#include "npy_pycompat.h"
#include "abstract.h"

#include "numpy/npy_math.h"
Expand Down Expand Up @@ -303,29 +304,35 @@ int initumath(PyObject *m)
* TODO: This should probably be done at a better place, or even in the
* code generator directly.
*/
s = _PyDict_GetItemStringWithError(d, "logical_and");
if (s == NULL) {
int res = PyDict_GetItemStringRef(d, "logical_and", &s);
if (res <= 0) {
return -1;
}
if (install_logical_ufunc_promoter(s) < 0) {
Py_DECREF(s);
return -1;
}
Py_DECREF(s);

s = _PyDict_GetItemStringWithError(d, "logical_or");
if (s == NULL) {
res = PyDict_GetItemStringRef(d, "logical_or", &s);
if (res <= 0) {
return -1;
}
if (install_logical_ufunc_promoter(s) < 0) {
Py_DECREF(s);
return -1;
}
Py_DECREF(s);

s = _PyDict_GetItemStringWithError(d, "logical_xor");
if (s == NULL) {
res = PyDict_GetItemStringRef(d, "logical_xor", &s);
if (res <= 0) {
return -1;
}
if (install_logical_ufunc_promoter(s) < 0) {
Py_DECREF(s);
return -1;
}
Py_DECREF(s);

if (init_string_ufuncs(d) < 0) {
return -1;
Expand Down
0