Add error returns to maxGridDiskSize#551
Conversation
|
|
||
| H3Index *out = malloc(H3_EXPORT(maxGridDiskSize)(40) * sizeof(H3Index)); | ||
| int64_t outSz; | ||
| H3_EXPORT(maxGridDiskSize)(40, &outSz); |
There was a problem hiding this comment.
Should the benchmarks check for the error condition?
There was a problem hiding this comment.
Updated for the setup part. I didn't feel the benchmark should check this as part of what it times since this isn't part of the code we're intending to measure.
dfellis
left a comment
There was a problem hiding this comment.
Only a minor comment on the validity of benchmarks that all fallible functions but assume correctness.
| if (k < 0) { | ||
| return E_DOMAIN; | ||
| } | ||
| *out = 3 * (int64_t)k * ((int64_t)k + 1) + 1; |
There was a problem hiding this comment.
Would it make sense to check somewhere that this value is less than the max number of cells at the current res? E.g. should kRing(res0cell, 100) throw E_DOMAIN?
There was a problem hiding this comment.
I have a concern that in some cases this will overflow, in that case we should consider returning the maximum number of cells?
There was a problem hiding this comment.
Yeah, that's probably better than returning an error (and should be correct, in that I think k-rings covering more than the total number of cells should return all cells at that res, right?).
We should be able to check a constant to avoid integer overflow, right?
|
One minor comment: Should we standardize the type we use for distance? Here, it looks like we're using h3/src/h3lib/include/h3api.h.in Lines 701 to 702 in cd47015 |
This PR also changes the signature for this function to support
kvalues where the number of cells is more than anint32_tcan represent (i.e.k=26755, maxGridDiskSize(26755)=2147570341)