BUG: Fix decref before incref for in-place accumulate #7407
Conversation
| dataptr_copy[0] = dataptr[0]; | ||
| dataptr_copy[1] = dataptr[1]; | ||
| dataptr_copy[2] = dataptr[0]; | ||
|
|
||
| /* Copy the first element to start the reduction */ | ||
| if (otype == NPY_OBJECT) { | ||
| Py_XINCREF(*(PyObject **)dataptr_copy[1]); | ||
| Py_XDECREF(*(PyObject **)dataptr_copy[0]); |
There was a problem hiding this comment.
So to check if I understand: what's going on here is, we're doing an accumulate, dataptr_copy[0] points to the out array, dataptr_copy[1] points to the in array, and when out is in then bad stuff can happen. In that case I think this logic is now correct, though it is very tricky :-).
Observation 1: this desperately needs a comment :-).
Observation 2: could we instead replace this whole if/else stuff with a call to the dtype's data copy method? that would let us remove the special case, we'll eventually need it when dtypes get smarter, and object dtype method should already handle this case?
|
LGTM, but agree with @njsmith that a comment would be useful. |
|
@seberg I want to put this in and backport for 1.11 (obviously correct), but a comment as to why the order is important would be very helpful. |
|
Added a (maybe a bit large) comment. I guess there is a point, Nathaniel is right about replacing these things probably, though maybe it would be better to replace more then just in this one place then as well. |
|
Looks good pending AppVeyor. A shorter comment would be something like |
|
I would expand "need to incref" to "need to incref before decref", but I like the current comment, it seems to give the right amount of information to me and we're not really in danger of leaving this code over-commented :-) |
|
Heh, yeah, at the very least a "first" is missing, will be added in a minute or two ;) |
|
OK, just left the long comment for now, true, Chucks version is nice too ;). |
BUG: Fix decref before incref for in-place accumulate
|
Thanks Sebastian. Nice find also. |
This is a follow up to numpy#7407 and numpy#7466. It fixes an error in the comments (0 is the output and 1 is the input), moves them around so they apply to both branches of the if/else, and changes memcpy to memmove, since the memory segments can be overlapping, as pointed out by Valgrind in a really old issue.
closes gh-7402