Add error return codes to gridDisk functions #505
Conversation
|
There may be some unreachable branches in this PR, I will see if I can get coverage on them or otherwise deduplicate some of the error handling. |
src/h3lib/lib/algos.c
Outdated
| // TODO: The return value, possibly indicating an error, is discarded here - | ||
| // we should use this when we update the API to return a value | ||
| normalizeMultiPolygon(out); |
There was a problem hiding this comment.
Should this function also be updated so it can return the error here?
|
We also need to update the docs for gridDisk. |
|
This should be ready for review. I began updating some additional functions in |
| // Optimistically try the faster gridDiskUnsafe algorithm first | ||
| const bool failed = | ||
| const H3Error failed = | ||
| H3_EXPORT(gridDiskDistancesUnsafe)(origin, k, out, distances); | ||
| if (failed) { |
There was a problem hiding this comment.
Is E_SUCCESS supposed to be opaque? If so, maybe if (result != E_SUCCESS) would be better than assuming 0.
Either way, we should be consistent with our internal handling here - either always use falsy checks or always use inequality checks.
There was a problem hiding this comment.
I think we are tied to E_SUCCESS == 0 and non-zero being an error, so my suggestion would be to test for any non-zero value.
There was a problem hiding this comment.
Sure that sounds good. In that case, there are a lot of if (result != E_SUCCESS) checks we should probably update. I'd also consider getting consistent around naming - maybe error rather than failed, e.g.
| H3Index h3NeighborRotations(H3Index origin, Direction dir, int *rotations) { | ||
| H3Index out = origin; | ||
| H3Error h3NeighborRotations(H3Index origin, Direction dir, int *rotations, | ||
| H3Index *out) { |
There was a problem hiding this comment.
Nit: I'd consider swapping the order of int *rotations, H3Index *out in the arg list. rotations is also an "out" argument, but it's less important than out, and in other languages you'd make it optional, which suggests that it should go last in the arg list. Not a big deal though.
There was a problem hiding this comment.
rotations is both an in and out parameter, which makes it a little trickier. Does that make this ordering make more sense or does it still seem of lesser importance?
There was a problem hiding this comment.
Hm. I missed that we were using rotations for more than reporting to the caller. Many of the callsites pre-set this to 0, but I guess gridDiskUnsafe and gridRingUnsafe use this as a real input param. Probably stet in this case.
| *rotations = *rotations + 5; | ||
| } else { | ||
| // Should never occur | ||
| return H3_NULL; // LCOV_EXCL_LINE | ||
| return E_FAILED; // LCOV_EXCL_LINE |
There was a problem hiding this comment.
I suspect that the coverage drops are because this is the only failure case for this function, and we've added a few if (neighborResult != E_SUCCESS) branches after calls to this function.
It looks like we might be able to force failure here with an invalid index, with an INVALID_DIGIT setting as the leading non-zero digit?
src/h3lib/lib/algos.c
Outdated
| origin, NEXT_RING_DIRECTION, &rotations, &origin); | ||
| if (neighborResult) { | ||
| return neighborResult; | ||
| } | ||
| if (origin == 0) { // LCOV_EXCL_BR_LINE |
There was a problem hiding this comment.
Nit: H3_NULL? Also, would h3NeighborRotations ever set out to H3_NULL now, or is this check meaningless?
There was a problem hiding this comment.
I think you are correct that we should update h3NeighborRotations docs and callsites to not look for H3_NULL.
src/h3lib/lib/algos.c
Outdated
| @@ -395,27 +406,28 @@ H3Index h3NeighborRotations(H3Index origin, Direction dir, int *rotations) { | |||
| // base cell. | |||
| if (oldLeadingDigit == CENTER_DIGIT) { | |||
| // Undefined: the k direction is deleted from here | |||
| return H3_NULL; | |||
| return E_SUCCESS; | |||
There was a problem hiding this comment.
This doesn't look right - out is not set in this case
uber/h3#505 updated this function to have a different signature; I assume this wasn't caught as a build error because the fuzzer is calling an internal function of the library that isn't in the `h3api.h` header.
uber/h3#505 updated this function to have a different signature; I assume this wasn't caught as a build error because the fuzzer is calling an internal function of the library that isn't in the `h3api.h` header.
Adds the ability for the gridDisk series of functions (but not
maxGridDiskSize, which may need to be updated to return int64_t) to return errors.