MAINT: Move ctype -> dtype conversion to python #12254
Conversation
96b6c79 to
b36a70c
Compare
9587794 to
fc1fe5a
Compare
|
The mystery travis warning machine strikes again... |
fc1fe5a to
c4bf24d
Compare
c4bf24d to
0820813
Compare
| _fields_ = [ | ||
| ('a', ctypes.c_uint8, 7), | ||
| ('b', ctypes.c_uint8, 1) | ||
| ] |
There was a problem hiding this comment.
Previously this produced dtype([('a', 'u1', (7,)), ('b', 'u1')], align=True), which was garbage.
| _fields_ = [ | ||
| ('a', ctypes.c_uint8), | ||
| ('b', ctypes.c_uint16), | ||
| ] |
There was a problem hiding this comment.
And this one produced dtype({'names':['a','b'], 'formats':['u1','<u2'], 'offsets':[0,2], 'itemsize':4}, align=True), which is not a union
numpy/core/src/common/npy_ctypes.h
Outdated
|
|
||
| #include "npy_import.h" | ||
|
|
||
| /*! |
There was a problem hiding this comment.
Yeah, I think it's doxygen. Copied from npy_import, but it seems we don't use it elsewhere - removing
| @@ -796,13 +796,13 @@ def _ufunc_doc_signature_formatter(ufunc): | |||
| ) | |||
|
|
|||
|
|
|||
| def _is_from_ctypes(obj): | |||
| # determine if an object comes from ctypes, in order to work around | |||
| def npy_ctypes_check(cls): | |||
There was a problem hiding this comment.
OK, I see that the static type is checked in the C code.
| PyObject *dtypedescr; | ||
| PyArray_Descr *newdescr; | ||
| int ret; | ||
| static PyArray_Descr *_arraydescr_from_ctypes_type(PyTypeObject *o) { |
There was a problem hiding this comment.
C style, return type on separate line.
There was a problem hiding this comment.
'o' is not a good choice for variable name.
| } | ||
| PyErr_Clear(); | ||
| if (!PyObject_TypeCheck(res, &PyArrayDescr_Type)) { | ||
| // sanity check that dtype_from_ctypes_type returned the right type, |
There was a problem hiding this comment.
Theoretically, C++ style comments should be reserved until we go full C99, which will be in 1.17 after we drop Python 2.7.
| * type might override the dtype if numpy does not otherwise | ||
| * support it. | ||
| */ | ||
| if (npy_ctypes_check((PyTypeObject *)obj)) { |
There was a problem hiding this comment.
Generally prefer to check against value, especially as the error return is '0'.
There was a problem hiding this comment.
Or maybe NPY_SUCCEED as that seems to be the interface for this bunch of functions.
There was a problem hiding this comment.
I'd rather leave this as is, since it matches things like PyString_Check
| @@ -1423,10 +1387,20 @@ PyArray_DescrConverter(PyObject *obj, PyArray_Descr **at) | |||
| check_num = NPY_VOID; | |||
| } | |||
| else { | |||
| *at = _arraydescr_fromobj(obj); | |||
| *at = _arraydescr_from_dtype_attr(obj); | |||
| if (*at) { | |||
There was a problem hiding this comment.
Error return here is NULL, be nice to use that in the error check.
There was a problem hiding this comment.
This whole function seems to use the style of not comparing against NULL, so I'd be inclined to leave it consistent
| fname, ftyp = item | ||
| fields.append((fname, dtype_from_ctypes_type(ftyp))) | ||
|
|
||
| # by default, ctypes structs are aligned |
There was a problem hiding this comment.
There's a _pack_ attribute that makes them otherwise: #10532
|
Some style nits. I'm going to rely for correctness on the existing and new tests. |
This comes with a number of changes: * Propagating the error if parsing a ctypes object goes wrong, rather than silencing it and trying more things. * Rejecting ctypes bitfields, which were incorrectly treated as subarrays. * Rejecting ctypes Unions, which were previously treated as if they were structs * Rejecting ctypes pointers, which were silently converted into their dereferenced types. This was particularly dangerous behavior when parsing structs with nested pointers, where swapping out the type changes the struct layout! * No longer support ctypes duck-types, which includes... * No longer supporting a `_fields_` attribute on `np.void` subclasses. This was a bug due to a poorly-shared code-path between void and ctypes types. The upshot is it should be easier to fix future problems in parsing ctypes types.
Without this, the build will not rerun due to changes in this file
0820813 to
0709d92
Compare
|
Thanks Eric. |
|
In hindsight, this probable needs a release note |
|
@eric-wieser @mattip This would be a very nice and useful feature since it is very common when dealing with data that originates in embedded systems. |
Follows in the vein of #10602.
A step towards fixing #12207.
This comes with a number of changes:
This was particularly dangerous behavior when parsing structs with nested pointers, where swapping out the type changes the struct layout!
_fields_attribute onnp.voidsubclasses. This was a bug due to a poorly-shared code-path between void and ctypes types.The upshot is it should be easier to fix future problems in parsing ctypes types.