BUG: Fix all kinds of problems when itemsize == 0 - #8977

the else clause and the new_data variable can be removed

What do you mean by removing the variable?

Fixing that feels kinda out of scope for this patch

IIRC, realloc will free the memory if size is zero, which we don't want to do. Worth checking, normally we keep a minimum of 1 byte to avoid problems.

Sometimes, yes - but we don't actually care. All that matters is if realloc returns NULL, (which realloc(..., 0) does on some platforms, in which case we fail with a memory error.

Either way, we don't need to worry about this yet - this function has a check for elsize == 0, which we shouldn't remove unless #8970 is merged.

So yes, technically, this file didn't need a patch at all - but I'd argue that npy_mul_with_overflow_intp does a better job of communicating the intention (which is why this change is in a MAINT commit)

this looks scetchy as the == 0 check is removed.
It might not go in here as then newbytes and oldnbytes are 0 but it's still dangerous

The idea here is that if the number of bytes are different, the element must be non empty. I guess rather than doing the division, we could just calculate this directly though

hm its probably good as is.

Patch incoming, I think it makes it cleaner

Also uncovered another unrelated bug

our usual style is having the operator on the line before but indenting the next line

That doesn't match with what I remember seeing, eg in mapping.c. Operators should be at the start of the line, so that they're not missed when reading through.

I can wrap after NPY_VOID if you'd like, but I'm not sure what level to indent to as a result

The usual style (Python recommended) is trailing operators and two indents on the the next line too separate the boolean expression from the body.

Is there something like CPEP 8 for cpython style guidelines?

You mean PEP7?

Also doc/C_STYLE_GUIDE.rst.txt. Here, second line just two indents from the if.

Already fixed here, I think

Can nbytes be -1? The inputs to npy_mul_with_overflow_int must be non-negative.

Huh, that's pretty weird. This couldn't happen before either, could it?

Why not?

For the same reason that np.dtype(np.void) is np.dtype(np.void) is desirable. Without this special case, v0_arr.view(np.void).dtype is not np.void.

Doesn't size need to be >= 0 in order to use npy_mul_with_overflow_intp? Seems it should be checked first to avoid a function call.

Could do something like

invalid = size < 0 || npy_mul_with_overflow_intp(...)

No need, there's already a explicit a != 0 case in that function.

There's no point avoiding the function call, because optimizing the performance of empty arrays is pretty pointless

Oh I see, the issue is size == -1. Maybe I'll just duplicate the below if.

Seems sensible, but why was it done differently before?

To avoid recalculating num, most likely

Could do

sd = newnbytes ? newnbytes : elsize;

Would prefer to keep the == 0, because that's handling the special meaning of 0 which is "allow resizing".

Could flatten the if though

newnbytes - oldnbytes >= 0?

Yep, guaranteed by the enclosing if

Is there an advantage to making this a staticmethod?

To document that it doesn't use self?

Well, it does that, don't know if it is worth it as the normal impulse would be to use it only if needed. Then again, my Python experience started when Python 21 was the newest and greatest, so programming conventions may have changed. In fact, it is the latter possibility that interests me. Do you know if it is the case that this sort of programming style is being taught or encouraged?

TestCase objects (or the inferred use of them by pytest) are pretty weird anyway, because an new one is instantiated for each method call by the test runner. I can change this if you feel strongly.

I don't feel strongly about it. On occasion I find your coding style a bit unusual, but not wrong. Just trying to figure out if it is a personal style, or the future of python coding ;)

Could easily be personal style. I guess it probably comes from my java background, but even there it might just have been personal style.