-
Notifications
You must be signed in to change notification settings - Fork 503
polygonToCells API accepts a flags argument #570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
836c504
to
00877d1
Compare
H3Index *hexagons = calloc(numHexagons, sizeof(H3Index)); | ||
|
||
resetMemoryCounters(0); | ||
failAlloc = true; | ||
H3Error err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, hexagons); | ||
H3Error err = H3_EXPORT(polygonToCells)(&sfGeoPolygon, 9, 0, hexagons); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Would prefer that tests use a const like DEFAULT_FLAGS
instead of 0
- it makes the call clearer and it will be easier to update when we have meaningful input to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value 0 will be part of the API so we will not be modifying the value of DEFAULT_FLAGS
. I can certainly add a constant for that. In the future when polygonToCells accepts more flags, we'll need to offer appropriate constants / defines for users to OR together.
int64_t *out) { | ||
uint32_t flags, int64_t *out) { | ||
if (flags != 0) { | ||
return E_FAILED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth adding E_UNSUPPORTED_FLAGS
or similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine either way. I don't believe the specific error code is considered part of the API contract so I don't see an issue with it changing.
This supersedes and includes #519 and #549, but includes only the API changes, but not any specific implementations.