-
Notifications
You must be signed in to change notification settings - Fork 522
Add isValidIndex function #1056
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
base: master
Are you sure you want to change the base?
Conversation
Implement a general validation function that checks if an H3 index is valid for any mode (cell, directed edge, or vertex). Changes: - Add isValidIndex() function declaration to h3api.h.in - Implement isValidIndex() in h3Index.c - Add includes for directedEdge.h and vertex.h in h3Index.c - Add comprehensive test coverage in testH3Index.c The function returns true if the index is valid as a cell, directed edge, or vertex, providing a single validation function for any H3 index type. Fixes uber#1043
src/apps/testapps/testH3Index.c
Outdated
// Test with valid directed edge | ||
H3Index neighbor; | ||
t_assertSuccess(H3_EXPORT(latLngToCell)(&coord, 5, &neighbor)); | ||
neighbor++; // Get a neighboring cell |
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.
This won't work. Simply incrementing will almost never give you a neighboring cell.
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.
@ajfriend I just pushed a commit to fix this. Thanks for the feedback. I used gridRingUnsafe to call the neighboring cells. Let me know what you think.
Replace incorrect neighbor++ with proper gridRingUnsafe call to get an actual neighboring cell for directed edge testing. Simply incrementing an H3Index will almost never produce a valid neighboring cell. This addresses the review feedback from ajfriend on lines 224-227.
Thanks for this! It looks like you may need to run/test this locally to get the formatting that the CI is expecting. |
Apply clang-format style to match project formatting conventions: - Put first two conditions on same line - Wrap long comment to 80 characters
@ajfriend happy to contribute. I fixed the formatting manually based on the logs then confirmed using clang-format. CI should hopefully pass now. |
Apply clang-format style to isValidIndex documentation comment: - Wrap comment across 3 lines for proper formatting
CI formatting failed on the header file, I re-formatted it with clang-format. |
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.
Thanks! Please note we will need to add this to fuzzers (e.g. https://github.com/uber/h3/blob/master/src/apps/fuzzers/fuzzerCellProperties.c) & docs as well.
Summary
Implements a general validation function
isValidIndex()
that checks if an H3 index is valid for any mode (cell, directed edge, or vertex), as requested in #1043.Changes Made
isValidIndex()
function declaration toh3api.h.in
h3Index.c
that checks all three validation types:directedEdge.h
andvertex.h
toh3Index.c
testH3Index.c
covering:API Usage
Test Plan
isValidIndex
intestH3Index.c
Fixes #1043