This is the actual change/hack. Instead of just using memset to initialize the buffer, copy the data from the original array into the buffer, so that decref'ing can occur. I could imagine that we can go somewhere earlier to a slow direct cast loop, which calls the cast function for every single element instead of attempting to speed it up by doing it in chunks (frankly, I doubt that helps for objects that need casting anyway).

OK, I think I finally understand what this is actually doing - it just checks whether any part is an object and then increases its refcount. It might not be bad to write a comment to state that, so that the next person looking at the code doesn't have to look up what PyArray_Item_INCREF actually does - the name is not too helpful here.

Separately, the check for object is very early on; I don't think there is much to gain by making a fast path, especially since anything to do with objects is slow anyway.

I was not 100% sure where you were going. Added some documentation in a new commit though.

As above, not clear to me this is worth the extra complexity.

Surely obj cannot be NULL here?

@seberg do you want to change this?

@mattip, I don't mind changing this. I am just not sure how much we implicitly support this, since NULL is also always checked e.g. in the ufunc loops that you worked on with the exception chaining.
But, I am pretty ambivalent about it. If you/anyone thinks we should just move away from it, lets do that. I do not really see the point in it.

Same, surely obj cannot be NULL (realize you probably copied from above, but it seems very odd).

I think you are right in principle. But IIRC quite a few, maybe even all, internal functions actually accept interpreting NULL as None for numpy arrays. In practice I do not think numpy itself will ever fill an array with NULL though, so the question would be if some extensions might actually rely on being able to fill arrays with NULL.

Here I'm a bit less sure, but the slow path would seem fast enough.

I timed it in this one and it was quite a big difference. On the other hand, probably almost nobody actually uses these features. And as soon as you do anything else, the actual speed is probably so slow, that it simply does not matter.

So I am good with just removing the fast paths, all of this code is pretty much only used doing creation and removal anyway.

+1 from me on removing this fast path.

This path isn't used in PyDataType_HASFIELDS - I don't see why we should optimize for it here either.

I think that

for(;;) {
   if (x) {
       y();
   }
   else {
      z();
   }
}

is clearer as

while (x()) {
   y();
}
z();

Might be good to elaborate a bit on this comment: is it numpy or python that is flaky with None?

Ooops, leftover for some earlier version, I think the test runs fine now.

Maybe I should call None also not None here, I got a feeling it helped for the others. I was wondering if it might have to do with the actual internal code python compiles too...

Doesn't help. It all seems to have to do with trickier things. Replacing the assert with a print for example runs it fine for me.

Frankly, I am not quite sure if the other tests (one and zero) are reliable or not. They seem to be, but..

Yikes...

More constructively: it would seem fine to just work with objects where we know nothing else can be increasing counts; maybe you are fighting some pytest internals...

This is wrong - the below refers to descr->subarray, which was previously guaranteed not to be null, but is no longer. Why is a reference check needed here?

I think perhaps you want (PyDataType_REFCHK(PyArray_DESCR(arr)) || swap) && PyArray_DESCR(arr)->subarray != NULL), which is better written as

// TODO: This doesn't seem safe for user-defined types. Is the optimization really worth it?
npy_bool skip_subarray_copy = !PyDataType_REFCHK(PyArray_DESCR(arr)) && !swap;

if (PyArray_DESCR(arr)->subarray != NULL && !skip_subarray_copy) {

Oh, moved the checks around. It is correct because of the early return in the previous if, but it reads strange like this...

Correct in the sense of the inital bug. But you are right, that was wrong. Also the subarray can be of VOID type again, so then you, again, need copyswap.

Added both with comments, I think it should be right now.

I still think the way I wrote it makes more sense...

Out of scope, but - line 295 will set an exception if an integer larger than 2**64 is passed in, which will probably lead to a SystemError

Frankly, I hope that this is only ever used for 0 or None, unfortunately it is public API.

It would be great to use pytest.mark.paramtrize here, rather than putting the for loop in the test. you might want to add a fourth label item to the tuple, so that you can do:

@pytest.mark.parametrize(['dt', 'pat', 'count', 'label'], iter_struct_object_dtypes(), id=lambda t: t[3])
def test_structured_object_create_delete(dt, pat, count, label):
    # don't actually use label here, pytest will have already handled it.

With the above suggestion, this becomes pytest.skip("None is flaky")

Probably more helpful to parametrize again.

@the_parametrize_I_used_in_the_comment_above
@pytest.mark.parametrize(['shape', 'indices', 'items_changed'], [
    ((3,), ([0, 2],), 2),
    ((3, 2), ([0, 2], slice(None)), 4),
    ((3, 2), ([0, 2], [1]), 2)
])
def test_structured_object_indexing(shape, indices, items_changed, dt, pat, count, label):

might be neat to introduce a helper object for this

class reference_counter:
    def __init__(self, obj):
        self._obj = obj
        self._initial_count = sys.getrefcount(obj)

    @property
    def change(self):
        return sys.getrefcount(self._obj) - self._initial_count


zero_counter = reference_counter(zero)
...
assert reference_counter.change == count * changed

Yeah, possibly. Anyway, did most (not all) of the parameterization, didn't use it much yet. If you got more, can fix it up a bit more, but probably won't continue otherwise.

Maybe should add a test for the non-contiguous subarray thing at least. But it is getting out of scope to start fixing that as well and getting late today at least.

Have to think about it. This test is good, but it spams valgrind, because uninitialized buffers are copied around. Maybe for now that is good. In the longer run – if we want to continue with such effort here – could add valgrind detection to the tests, which may be an option to skip it.

Could add a @pytest.mark.confuses_valgrind custom mark, which will run the test by default, but if run as valgrind python runtest.py -- -m "not confuses_valgrind" will skip it

Added pytest.mark.valgrind_error (it is a true error, not just confusion) and rebased.