[READY] Small fixes in numpy.h

[bstaletic]

• PyArray_NewFromDescr_ should have been using Py_intptr_t const *
  • https://github.com/numpy/numpy/blob/18a6e3e505ee416ddfc617f3e9afdff5a031c2c2/numpy/core/src/multiarray/ctors.h#L5-L8
• PyArray_GetArrayParamsFromObject_ should be using NPY_BOOL
  • https://github.com/numpy/numpy/blob/18a6e3e505ee416ddfc617f3e9afdff5a031c2c2/numpy/core/src/multiarray/ctors.c#L1350-L1355
  • https://docs.scipy.org/doc/numpy-1.13.0/reference/c-api.dtype.html#c.npy_bool
• PyArray_DescrNewFromType is at offset 96
  • https://github.com/numpy/numpy/blob/36e017194c32a53cf2965a513243cb3c348095df/numpy/core/code_generators/numpy_api.py#L141
• array_t constructor that takes a size and a pointer should take ssize_t.
  • Fixes array_t constructor takes shape as size_t, not ssize_t #1599

[YannickJadoul]

PyArray_DescrNewFromType is at offset 96

How was this never noticed? This means that PyArray_DescrNewFromType is never used?

array_t constructor that takes a size and a pointer should take ssize_t.

This was changed in #782, but it seems that was a bit too enthusiast? Would you mind going over the other changes in this PR and see if more things that should not made signed now still are?

[YannickJadoul]

array_t constructor that takes a size and a pointer should take ssize_t.

This was changed in #782, but it seems that was a bit too enthusiast? Would you mind going over the other changes in this PR and see if more things that should not made signed now still are?

There has actually been discussion on that: #782 (review)

[bstaletic]

PyArray_DescrNewFromType is at offset 96

How was this never noticed? This means that PyArray_DescrNewFromType is never used?

It is never used. Also, the compiler has no way of warning us here because this works by grabbing a capsule, casting it to a void** and then using offsets to cast void* to function pointers. It completely side-steps any possibility for type safety.

array_t constructor that takes a size and a pointer should take ssize_t.

This was changed in #782, but it seems that was a bit too enthusiast? Would you mind going over the other changes in this PR and see if more things that should not made signed now still are?

There has actually been discussion on that: #782 (review)

Hmm... but we're geting a cast to signed anyway. That's why the warning happens.

[YannickJadoul]

It is never used. Also, the compiler has no way of warning us here because this works by grabbing a capsule, casting it to a void** and then using offsets to cast void* to function pointers. It completely side-steps any possibility for type safety.

Right, I thought so. Should we maybe comment out the things that aren't used (and not tested)?

Hmm... but we're geting a cast to signed anyway. That's why the warning happens.

Ah, wait, you're changing it tó ssize_t. Right, this was forgotten in the array_t, but I was looking at array. (In my defence, that's only 2 bytes difference.)

[bstaletic]

It is never used. Also, the compiler has no way of warning us here because this works by grabbing a capsule, casting it to a void** and then using offsets to cast void* to function pointers. It completely side-steps any possibility for type safety.

Right, I thought so. Should we maybe comment out the things that aren't used (and not tested)?

I'd rather drop it completely than comment it out. As this pull request shows, the more of numpy's multiarray C API we use, the worse. Should I check the rest and remove all that aren't used?

[bstaletic]

I'd rather drop it completely than comment it out. As this pull request shows, the more of numpy's multiarray C API we use, the worse. Should I check the rest and remove all that aren't used?

After a short discussion with @YannickJadoul we agreed not to touch PyArray_DescrNewFromType besides fixing the offset, because removing it would affect the ABI of py::detail::npy_api.

[wjakob]

Oh gosh, good catch about the bad offset. All of these changes look good to me, please feel free to merge yourself when ready.

[YannickJadoul]

All is green; let's go!

[YannickJadoul]

Thanks, @bstaletic!