Fix C API mismatches vs. libspatialindex #222
Conversation
In the ctypes binding for Index_CreateWithStream, change the last parameter from ctypes.POINTER(ctypes.c_uint32) to ctypes.POINTER(ctypes.c_size_t) for consistency with libspatialindex. This fixes the test failure on 64-bit big-endian architectures that prompted Toblerity#220. Other inconsistencies noted in that issue will be fixed in additional commits. Toblerity#220 (comment)
In the libspatialite API, Error_GetLastErrorNum, Error_GetLastErrorMethod, and Error_GetErrorCount all take no parameters “…(void)”. Add “….argtypes = []” to the ctypes bindings for Error_GetLastErrorNum and Error_GetLastErrorMethod to match the other two. This tells ctypes that they should have no parameters, “…(void)” in C, rather than unspecified parameters, “…()” in C.' Toblerity#220 (comment)
In SIDX_NewBuffer and Index_InsertTPData bindings, fix use of c_uint (unsigned int) or c_uint32 (uint32_t) for value parameters that should be c_size_t (size_t) for consistency with the libspatialite C API. Toblerity#220 (comment)
Change IndexProperty_SetIndexType to use c_int instead of c_int32 for the “enum RTIndexType” parameter, and change IndexProperty_SetIndexVariant and IndexProperty_SetIndexStorage to use c_int instead of c_uint32 for their “enum RTIndexVariant” and “enum RTStorageType” parameters, respectively. This aligns these prototypes with the prevailing of c_int in these bindings to wrap enums, elsewhere in the bindings for these particular enum types, and even in the getters corresponding to these three particular setters. Toblerity#220 (comment)
Fix a number of ctypes bindings for index property getters in which the return type was given as c_int (int), but the libspatialite API has c_uint32 (uint32_t). Toblerity#220 (comment)
Add argument types and error checking callback. Toblerity#220 (comment)
|
Tests aren't happy here with these changes quite yet. |
I noticed that, but haven’t had a chance to look into it yet. Since it Works on My Machine™ with spatialindex 1.9.3, I’ll have to take a little time to get the bottom of it. Strangely, the two failing tests are the same ones that prompted #220. |
|
I went ahead and pushed a commit that should fix this; see #220 (comment). I’d rather not have to set up a local environment with libspatialite 1.8.5; if the tests pass this time, then I can avoid that. That said, I’m very much open to alternative approaches. |
|
Looks like a few lint things still, but this is converging |
musicinmybrain
force-pushed
the
issue-220
branch
from
c66c74b
to
5852fe2
Compare
|
Force-pushed with the most recent commit “blackened”. I haven’t used mypy yet, but it looks like the initial assignment _nDataLength_type = ctypes.POINTER(ctypes.c_size_t)locks in the type, and then conditionally reassigning _nDataLength_type = ctypes.POINTER(ctypes.c_uint32)is “wrong.” I… didn’t expect it would work that way. I can rearrange the logic to make |
musicinmybrain
force-pushed
the
issue-220
branch
from
5852fe2
to
39674bb
Compare
|
Force-pushed again; |
|
Is this ready to merge once it is ✅ ? |
Yep, this is correct. Python is dynamically typed, but mypy can only check the type for statically typed objects. So changing the type of a variable is considered "bad" by mypy, but you can always specify the type as |
As far as I can tell, yes. |
This fixes #220 by correcting the mismatches with the
libspatialindexAPI that were noted in that issue and its comments.