Remove c casts#66
Conversation
| if (i > std::numeric_limits<INDEX>::max()) | ||
| { | ||
| PyErr_Format(PyExc_ValueError, "Combine1Filter has attempted to exceed the size of the index value [%lld]", i); | ||
| return 0; |
There was a problem hiding this comment.
I'm curious - will this actually cause the exception to be raised when returning to Python?
There was a problem hiding this comment.
I don't think this would work, or if it did it seems rather obfuscated.
I think a better pattern may be to return an (result,error) tuple, so the root function could return the error.
@jack-pappas do you have a better suggestion?
There was a problem hiding this comment.
To be honest I don't know. It seemed the most python-esque error reporting style in use in the file.
There was a problem hiding this comment.
I think the general pattern we want is to have only the publicly-exposed (as a function in the riptide_cpp Python module) setting the error/exception for a couple reasons:
- That pattern makes it easy to know which function has the responsibility for handling error and/or setting the Python exception.
- I think we want to be careful to keep the codebase generally pure C++ (that doesn't know about Python), with a "wrapper" that exposes the core C++ code to Python and handles the interop.
There was a problem hiding this comment.
So two options then: error return (caller must check+translate) or throw exception (and caller must catch+translate)...
I lean towards exception, but I know code-base isn't very exception friendly.
There was a problem hiding this comment.
I think the general pattern we want is to have only the publicly-exposed (as a function in the
riptide_cppPython module) setting the error/exception for a couple reasons:
- That pattern makes it easy to know which function has the responsibility for handling error and/or setting the Python exception.
This makes sense to me.
- I think we want to be careful to keep the codebase generally pure C++ (that doesn't know about Python), with a "wrapper" that exposes the core C++ code to Python and handles the interop.
Absolutely. That's the main reason for the namespace split in the variant based refactor. The benefits are numerous, and the niggles fortunately few (at least when refactoring).
There was a problem hiding this comment.
So two options then:
error return (caller must check+translate)
or throw exception (and caller must catch+translate)...
I lean towards exception, but I know code-base isn't very exception friendly.
Either causes a refactor around the call site. I'll take a look and see what feels cleanest. It may be the excuse to create our own exception base class so that we have something just-specific-enough to catch.
There was a problem hiding this comment.
Throw up to a layer that already had the PyErr_Format / return NULL logic in place implemented.
| // partStart, partLength, uniquesBefore); | ||
|
|
||
| if (callbackArg->sizeofUINDEX == 4) | ||
| if constexpr (sizeof(UINDEX) == 4) |
There was a problem hiding this comment.
If you're making this change generally, it seems like the sizeofUINDEX field of the struct above (on line 4078) could be removed altogether?
There was a problem hiding this comment.
That does feel like a good idea. This was the only one whose side-effects were causing compiler warnings, so I didn't catch the rest.
|
|
||
| int ndimOut; | ||
| int64_t strideOut; | ||
| int32_t strideOut; |
There was a problem hiding this comment.
Shouldn't we be using npy_intp or intptr_t (or ptrdiff_t) for representing the stride size everywhere? That's what's returned by the numpy C APIs when asking for the strides of an ndarray.
There was a problem hiding this comment.
ptrdiff_t is the correct type according to standard-ese (the size required to hold the difference between an arbitrary pair of pointers within the same object).
However, I suspect that what we really have is an afine space of index and stride - so we should really make strong types out of both of them and subset their operations accordingly.
This changeset is rather in the way of an experiment to see what would happen if we restrict strides to 32-bit values, to avoid having to change the SIMD stride math UnaryOps and TileRepeat do. I wonder if we can live with "current behaviour" (ie re-instating various static_cast's) until we get the variant refactor for those done.
…tions. Also some slight re-naming to distinguish single, from multiple, types
34e6ed9 to
f00a4ee
Compare
|
This is an exposition-only PR. We've pulled some ideas from it, and will leave it closed so that we can pull further ideas as we need to. |
3 participants
An attempt to clear up what happens if we removed all the (u)int32_t C-style casts in the codebase. In some cases the clean-up is no doubt the ideal way to deal with the local issue, and in some cases I've punted and tried to enforce a reasonable range. It will in other words probably need a bit of spit and polish were we to merge it as it currently stands.