API: Introduce PyDataType_FLAGS accessor for public access
#25816
Conversation
The flags are too small at 8 bits, so create a new uint64 accessor function for public use. Internally, we can keep using flags just fine (since we don't need version specific handling). Cython 2 will end up going through python to get the flags so make a note of that in the release notes.
seberg
changed the title
PyDataType_FLAGS accessor for public accessPyDataType_FLAGS accessor for public access
seberg
force-pushed
the
descr-flags-accessor
branch
2 times, most recently
from
43c8d78
to
c754f2a
Compare
The need to declare static inline functions which are then defined in `npy_2_compat.h` means that we must make sure `npy_2_compat.h` actually gets included always to avoid some compilers to error.
seberg
force-pushed
the
descr-flags-accessor
branch
from
c754f2a
to
d90cbd1
Compare
|
The last commit is one slight wrinkle, but I don't think it is too bad. Because these functions need runtime dependent behavior, they cannot be reasonably used in |
|
Note that this also serves as a blueprint for changing |
| @@ -0,0 +1,10 @@ | |||
| * Due to runtime dependencies, some functionality was moved out of | |||
| ``numpy/ndarraytypes.h`` and is only available after including ``ndarrayobject.h`` | |||
There was a problem hiding this comment.
It's not clear which functionality you're talking about. Can you rephrase to make this first sentence less ambiguous?
| static inline npy_uint64 | ||
| PyDataType_FLAGS(const PyArray_Descr *dtype) | ||
| { | ||
| return dtype->flags; |
There was a problem hiding this comment.
I think this needs an explicit cast to npy_uint64, because this is a signed char on numpy 1.x and might be negative?
There was a problem hiding this comment.
I thought the return value will do the trick, but maybe better safe then sorry.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return ((PyArray_Descr *)dtype)->flags; | ||
| } | ||
| else { | ||
| return ((PyArray_DescrProto *)dtype)->flags; |
There was a problem hiding this comment.
I think you need an explicit cast here as well.
Co-authored-by: Nathan Goldbaum <nathan.goldbaum@gmail.com>
There was a problem hiding this comment.
I just did some searching to judge the downstream impact of this change, and I think we're fine, at least as far as public code goes. I see a few uses of some of these macros in old docs and in a couple of unmaintained packages, but nothing in the latest version of any major reverse dependency (some usage in old versions of pandas though).
So I think this one might be low risk, although I wouldn't be surprised if there is a long tail of users of some of these macros that don't show up on github searches.
It would ameliorate my concern about this if we could get a mention in the migration guide to deal with any immediate fallout after this is merged, since people might miss release note fragments.
|
I'm going to merge this one now. There's a note to update the migration guide in the followup tracking issue. |
The flags are too small at 8 bits, so create a new uint64 accessor function for public use. Internally, we can keep using flags just fine (since we don't need version specific handling).
Cython 2 will end up going through python to get the flags so make a note of that in the release notes.
Another relatively small preparatory step for opening up
PyArray_Descrto serious changes. This moves flags specifically to be an inline function and exposes it as such to Cython.Note: I wanted to use
npy_uint64(up for discussion), which is why I had to move the type definition block to the beginning. But that seemed very reasonable in either case.