Repository navigation
Fix CLI cell lists dropping cells near the end of the input - #1276
unknownhackerworld wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Expand CLI cell-list capacity src/apps/filters/h3.c |
The compactCells, uncompactCells, and cellsToMultiPolygon commands now use a shared 1,701-byte buffer and scan up to 1,700 characters. |
Scan input through its final cell src/apps/filters/h3.c, tests/cli/uncompactCells.txt, tests/inputfiles/uncompact_test4.txt, tests/inputfiles/uncompact_test5.txt, CHANGELOG.md |
readCellsFromFile preserves split cells across streamed chunks and scans the final tail at EOF. CLI tests cover 100-cell argument input and file and stdin input where the final cell falls within the last 15 bytes. The changelog records the correction. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Low
Merge Risk: 
7f8e8
-
The changed CLI paths cover the reported 100-cell and trailing-file-cell cases. No issue identified here needs resolution before merge.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title names the main change: the CLI subcommands (compactCells, uncompactCells, cellsToMultiPolygon) no longer drop cells near the end of the input. The changeset adds a larger --cells buffer and … Description check The description states the problem (cells dropped by the --cells buffer limit and by the readCellsFromFile tail handling), the root cause, the fix (new 1701-byte buffer with %1700c, strlen-based scan … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title names the main change: the CLI subcommands (compactCells, uncompactCells, cellsToMultiPolygon) no longer drop cells near the end of the input. The changeset adds a larger --cells buffer and fixes readCellsFromFile so the carried-over tail is scanned, which matches the title.
Full details: Description check
-
Explanation
The description states the problem (cells dropped by the --cells buffer limit and by the readCellsFromFile tail handling), the root cause, the fix (new 1701-byte buffer with %1700c, strlen-based scan for strings, tail scan at EOF), tradeoffs, and test results. It matches the changeset, which includes h3.c, the CHANGELOG entry, and the new CLI tests and input files.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
|
|
coverage: 99.758%. remained the same — unknownhackerworld:fix/cli-cells-near-end into uber:master |
Signed-off-by: Allen Joseph G <63872540+unknownhackerworld@users.noreply.github.com>
7cecbb5 to
7f8e848
Compare
Summary
The
compactCells,uncompactCellsandcellsToMultiPolygonCLI subcommands silently drop cells in two cases:--cellslist is read with%1500cinto a 1501 byte buffer, but the help text says it takes "Up to 100 cells". 100 cells separated by,take 1598 characters, so everything after character 1500 is cut off with no error.readCellsFromFileonly scans up toBUFFER_SIZE_LESS_CELL(1485) in every chunk, including the last one. A cell that starts in the last 15 bytes of the input is never read. This affects the-cstring (which has no following chunk) and the final chunk of a file or stdin at EOF when that chunk fills the buffer.Root cause
src/apps/filters/h3.c:compactCells,uncompactCellsandcellsToMultiPolygondeclarechar cellStrs[BUFFER_SIZE_WITH_NULL]with.scanFormat = "%1500c". The size was shared with the file read buffer, but a-clist of 100 cells needs 100 * 15 + 99 * 2 = 1698 characters.readCellsFromFile, the inner loop iswhile (bufferOffset < BUFFER_SIZE_LESS_CELL). Holding back the last 15 bytes is right when more of the file follows, since a cell split across reads gets carried over and reassembled. But whenfp == NULL, the loop runs once and stops. And at EOF, thedo { ... } while (bytesRead > 0)loop exits after the carried-over tail was moved to the front of the buffer, so the tail is never scanned.Two branches in the function could not run: the
cell == 0 && bufferOffset == BUFFER_SIZEreset (the loop condition stops at 1485), and thefp == NULLbranch aroundfread(thefp == NULLcase never reaches it). This PR removes both.Fix
CELL_LIST_ARG_SIZE_WITH_NULL(1701) and use it with%1700cfor the-cbuffers in the three subcommands. 1700 leaves room for 100 cells with,separators.readCellsFromFile, track whether the current chunk is the last one:fp == NULL, the buffer is a NUL-terminated string. It is scanned tostrlen(buffer)and the loop ends.freadreturns 0, the carried-over tail is scanned one last time up totailLen.Tradeoffs and alternatives
-cbuffer and only made it large enough for the documented 100 cells. Allocating it dynamically would remove the limit, butparseArgsscans into a fixed buffer throughscanFormat, so that would be a larger change to the arg parser.-clist longer than 1700 characters is still cut off silently, same as before for lists longer than 1500. Raising an error there seemed out of scope.Testing
New CLI tests in
tests/cli/uncompactCells.txtwith two new input files:tests/inputfiles/uncompact_test4.txt: 100 resolution 1 cells separated by,(1698 characters plus a newline).tests/inputfiles/uncompact_test5.txt: exactly 1500 bytes of newline separated cells with no trailing newline, so the last cell starts at offset 1485.Results with
uncompactCells -r 1 ... -f newline | wc -l:-c "$(cat uncompact_test4.txt)"-i uncompact_test5.txt-i -- < uncompact_test5.txtThe new tests fail on master and pass with this change. All 322 ctest tests pass locally, and
clang-format14 reports no changes.AI disclosure: this change was found, written and tested with help from an AI assistant.