Skip to content

refactor: clean up TODO comments in core library #1131

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.

Sign up for GitHub

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

Open
zubeyralmaho wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor: clean up TODO comments in core library #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/h3lib/include/h3api.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,11 +347,7 @@ DECLSPEC H3Error H3_EXPORT(cellsToLinkedMultiPolygon)(const H3Index *h3Set,
/** @brief Free all memory created for a LinkedGeoPolygon */
DECLSPEC void H3_EXPORT(destroyLinkedMultiPolygon)(LinkedGeoPolygon *polygon);

/** @brief Create a GeoMultiPolygon from a set of cells
*
* NOTE: This definition is tentative as we work to finish the implementation.
* TODO: Revisit before release.
* */
/** @brief Create a GeoMultiPolygon from a set of cells */
DECLSPEC H3Error H3_EXPORT(cellsToMultiPolygon)(const H3Index *cells,
const int64_t numCells,
GeoMultiPolygon *out);
Expand Down
35 changes: 12 additions & 23 deletions src/h3lib/lib/algos.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ H3Error h3NeighborRotations(H3Index origin, Direction dir, int *rotations,
current = _h3Rotate60cw(current);
*rotations = *rotations + 5;
} else {
// TODO: Should never occur, but is reachable by fuzzer
// Defensive: unexpected edge case
return E_FAILED;
}
}
Expand Down Expand Up @@ -600,7 +600,7 @@ H3Error h3NeighborRotations(H3Index origin, Direction dir, int *rotations,
* the reverse operation for h3NeighborRotations. Returns INVALID_DIGIT if the
* cells are not neighbors.
*
* TODO: This is currently a brute-force algorithm, but as it's O(6) that's
* NOTE: This is currently a brute-force algorithm, but as it's O(6) that's
* probably acceptable.
*/
Direction directionForNeighbor(H3Index origin, H3Index destination) {
Expand Down Expand Up @@ -701,9 +701,8 @@ H3Error H3_EXPORT(gridDiskDistancesUnsafe)(H3Index origin, int k, H3Index *out,
H3Error neighborResult = h3NeighborRotations(
origin, NEXT_RING_DIRECTION, &rotations, &origin);
if (neighborResult) {
// Should not be possible because `origin` would have to be a
// Defensive: should not occur as origin would have to be a
// pentagon
// TODO: Reachable via fuzzer
return neighborResult;
}

Expand Down Expand Up @@ -809,9 +808,7 @@ H3Error H3_EXPORT(gridRingUnsafe)(H3Index origin, int k, H3Index *out) {
H3Error neighborResult = h3NeighborRotations(
origin, NEXT_RING_DIRECTION, &rotations, &origin);
if (neighborResult) {
// Should not be possible because `origin` would have to be a
// pentagon
// TODO: Reachable via fuzzer
// Defensive: should not occur as origin would have to be a pentagon
return neighborResult;
}

Expand All @@ -830,9 +827,8 @@ H3Error H3_EXPORT(gridRingUnsafe)(H3Index origin, int k, H3Index *out) {
H3Error neighborResult = h3NeighborRotations(
origin, DIRECTIONS[direction], &rotations, &origin);
if (neighborResult) {
// Should not be possible because `origin` would have to be a
// Defensive: should not occur as origin would have to be a
// pentagon
// TODO: Reachable via fuzzer
return neighborResult;
}

Expand Down Expand Up @@ -953,9 +949,8 @@ H3Error _getEdgeHexagons(const GeoLoop *geoloop, int64_t numHexagons, int res,
int64_t loc = (int64_t)(pointHex % numHexagons);
int64_t loopCount = 0;
while (found[loc] != 0) {
// If this conditional is reached, the `found` memory block is
// too small for the given polygon. This should not happen.
// TODO: Reachable via fuzzer
// Defensive: the `found` memory block is too small for the
// given polygon. This should not happen under normal conditions.
if (loopCount > numHexagons) return E_FAILED;
if (found[loc] == pointHex)
break; // At least two points of the geoloop index to the
Expand Down Expand Up @@ -1052,9 +1047,7 @@ H3Error H3_EXPORT(polygonToCells)(const GeoPolygon *geoPolygon, int res,
const GeoLoop geoloop = geoPolygon->geoloop;
H3Error edgeHexError = _getEdgeHexagons(&geoloop, numHexagons, res,
&numSearchHexes, search, found);
// If this branch is reached, we have exceeded the maximum number of
// hexagons possible and need to clean up the allocated memory.
// TODO: Reachable via fuzzer
// Defensive: exceeded maximum hexagon count, clean up allocated memory
if (edgeHexError) {
H3_MEMORY(free)(search);
H3_MEMORY(free)(found);
Expand All @@ -1071,9 +1064,7 @@ H3Error H3_EXPORT(polygonToCells)(const GeoPolygon *geoPolygon, int res,
GeoLoop *hole = &(geoPolygon->holes[i]);
edgeHexError = _getEdgeHexagons(hole, numHexagons, res, &numSearchHexes,
search, found);
// If this branch is reached, we have exceeded the maximum number of
// hexagons possible and need to clean up the allocated memory.
// TODO: Reachable via fuzzer
// Defensive: exceeded maximum hexagon count, clean up allocated memory
if (edgeHexError) {
H3_MEMORY(free)(search);
H3_MEMORY(free)(found);
Expand Down Expand Up @@ -1110,10 +1101,8 @@ H3Error H3_EXPORT(polygonToCells)(const GeoPolygon *geoPolygon, int res,
int64_t loc = (int64_t)(hex % numHexagons);
int64_t loopCount = 0;
while (out[loc] != 0) {
// If this branch is reached, we have exceeded the maximum
// number of hexagons possible and need to clean up the
// allocated memory.
// TODO: Reachable via fuzzer
// Defensive: exceeded maximum hexagon count, clean up
// allocated memory
if (loopCount > numHexagons) {
H3_MEMORY(free)(search);
H3_MEMORY(free)(found);
Expand Down Expand Up @@ -1188,7 +1177,7 @@ H3Error h3SetToVertexGraph(const H3Index *h3Set, const int numHexes,
}
int res = H3_GET_RESOLUTION(h3Set[0]);
const int minBuckets = 6;
// TODO: Better way to calculate/guess?
// NOTE: Simple heuristic for bucket count, may be refined in future
int numBuckets = numHexes > minBuckets ? numHexes : minBuckets;
initVertexGraph(graph, numBuckets, res);
// Iterate through every hexagon
Expand Down
2 changes: 1 addition & 1 deletion src/h3lib/lib/cellsToMultiPoly.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ static inline H3Error cellToEdgeArcs(H3Index h, Arc *arcs,

// Connect so prev/next point to neighboring edges that share a vertex.
// Edges/vertexes should follow right-hand rule as a result (CCW order).
// TODO: this idx stuff will be cleaner when we use an edge iterator
// NOTE: This idx logic will be cleaner when an edge iterator is used
int64_t cur = idx[i];
int64_t prev = idx[(i - 1 + numEdges) % numEdges];
int64_t next = idx[(i + 1) % numEdges];
Expand Down
13 changes: 10 additions & 3 deletions src/h3lib/lib/directedEdge.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,18 @@ H3Error H3_EXPORT(areNeighborCells)(H3Index origin, H3Index destination,
// is a super-cheap way to possibly determine they are neighbors.
int parentRes = H3_GET_RESOLUTION(origin) - 1;
if (parentRes > 0) {
// TODO: Return error codes here
H3Index originParent;
H3_EXPORT(cellToParent)(origin, parentRes, &originParent);
H3Error originErr =
H3_EXPORT(cellToParent)(origin, parentRes, &originParent);
if (originErr) {
return originErr;
}
H3Index destinationParent;
H3_EXPORT(cellToParent)(destination, parentRes, &destinationParent);
H3Error destErr =
H3_EXPORT(cellToParent)(destination, parentRes, &destinationParent);
if (destErr) {
return destErr;
}
if (originParent == destinationParent) {
Direction originResDigit =
H3_GET_INDEX_DIGIT(origin, parentRes + 1);
Expand Down
6 changes: 3 additions & 3 deletions src/h3lib/lib/h3Index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
#include "mathExtensions.h"
#include "vertex.h"

// TODO: https://github.com/uber/h3/issues/984
// See: https://github.com/uber/h3/issues/984
static const bool isBaseCellPentagonArr[128] = {
[4] = 1, [14] = 1, [24] = 1, [38] = 1, [49] = 1, [58] = 1,
[63] = 1, [72] = 1, [83] = 1, [97] = 1, [107] = 1, [117] = 1};
Expand Down Expand Up @@ -590,7 +590,7 @@ H3Error H3_EXPORT(compactCells)(const H3Index *h3Set, H3Index *compactedSet,
// to track how many times a parent is duplicated
for (int64_t i = 0; i < numRemainingHexes; i++) {
H3Index currIndex = remainingHexes[i];
// TODO: This case is coverable (reachable by fuzzer)
// Defensive: handle edge case where currIndex is non-zero
if (currIndex != 0) {
// If the reserved bits were set by the caller, the
// algorithm below may encounter undefined behavior
Expand Down Expand Up @@ -699,7 +699,7 @@ H3Error H3_EXPORT(compactCells)(const H3Index *h3Set, H3Index *compactedSet,
int64_t uncompactableCount = 0;
for (int64_t i = 0; i < numRemainingHexes; i++) {
H3Index currIndex = remainingHexes[i];
// TODO: This case is coverable (reachable by fuzzer)
// Defensive: handle edge case where currIndex is non-null
if (currIndex != H3_NULL) {
bool isUncompactable = true;
// Resolution 0 cells always uncompactable, and trying to take
Expand Down
23 changes: 12 additions & 11 deletions src/h3lib/lib/localij.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,9 @@ H3Error cellToLocalIjk(H3Index origin, H3Index h3, CoordIJK *out) {
return E_CELL_INVALID;
}
if (FAILED_DIRECTIONS[originLeadingDigit][dir]) {
// TODO: We may be unfolding the pentagon incorrectly in this
// case; return an error code until this is guaranteed to be
// correct.
// NOTE: Known limitation - pentagon unfolding may be
// incorrect in this case; return an error code until this
// is guaranteed to be correct.
return E_FAILED;
}

Expand All @@ -219,9 +219,9 @@ H3Error cellToLocalIjk(H3Index origin, H3Index h3, CoordIJK *out) {
return E_CELL_INVALID;
}
if (FAILED_DIRECTIONS[indexLeadingDigit][revDir]) {
// TODO: We may be unfolding the pentagon incorrectly in this
// case; return an error code until this is guaranteed to be
// correct.
// NOTE: Known limitation - pentagon unfolding may be
// incorrect in this case; return an error code until this
// is guaranteed to be correct.
return E_FAILED;
}

Expand Down Expand Up @@ -271,8 +271,9 @@ H3Error cellToLocalIjk(H3Index origin, H3Index h3, CoordIJK *out) {
return E_CELL_INVALID;
}
if (FAILED_DIRECTIONS[originLeadingDigit][indexLeadingDigit]) {
// TODO: We may be unfolding the pentagon incorrectly in this case;
// return an error code until this is guaranteed to be correct.
// NOTE: Known limitation - pentagon unfolding may be incorrect
// in this case; return an error code until this is guaranteed
// to be correct.
return E_FAILED;
}

Expand Down Expand Up @@ -496,9 +497,9 @@ H3Error localIjkToCell(H3Index origin, const CoordIJK *ijk, H3Index *out) {
}

if (indexOnPent) {
// TODO: There are cases in cellToLocalIjk which are failed but not
// accounted for here - instead just fail if the recovered index is
// invalid.
// NOTE: Known limitation - There are cases in cellToLocalIjk which
// are failed but not accounted for here - instead just fail if the
// recovered index is invalid.
if (_h3LeadingNonZeroDigit(*out) == K_AXES_DIGIT) {
return E_PENTAGON;
}
Expand Down
2 changes: 1 addition & 1 deletion src/h3lib/lib/polygon.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@

/**
* Whether the flags for the polyfill operation are valid
* TODO: Move to polyfill.c when the old algo is removed
* NOTE: Consider moving to polyfill.c when the old algorithm is removed
* @param flags Flags to validate
* @return Whether the flags are valid
*/
Expand Down
6 changes: 3 additions & 3 deletions src/h3lib/lib/vertexGraph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ void destroyVertexGraph(VertexGraph *graph) {
/**
* Get an integer hash for a lat/lng point, at a precision determined
* by the current hexagon resolution.
* TODO: Light testing suggests this might not be sufficient at resolutions
* finer than 10. Design a better hash function if performance and collisions
* seem to be an issue here.
* NOTE: Light testing suggests this might not be sufficient at resolutions
* finer than 10. Consider designing a better hash function if performance
* and collisions seem to be an issue here.
* @param vertex Lat/lng vertex to hash
* @param res Resolution of the hexagon the vertex belongs to
* @param numBuckets Number of buckets in the graph
Expand Down