Define the pentagon lookup table once (#984)

[phanisaimunipalli]

Partly addresses #984.

What this changes

h3Index.c carried a private isBaseCellPentagonArr copy of the pentagon flags, with a // TODO: https://github.com/uber/h3/issues/984 on it, while baseCells.c answered the same question from baseCellData[].isPentagon. Two hardcoded tables, nothing keeping them in sync.

This moves the table to baseCellIsPentagon in baseCells.c, declares it in baseCells.h, and points both _isBaseCellPentagon and the h3Index.c hot paths at it.

The array stays sized to NUM_BASE_CELL_VALUES (128) rather than NUM_BASE_CELLS (122), matching what the private array did. _hasDeletedSubsequence indexes it with a base cell number taken straight out of an index, and although both current callers bounds check first, shrinking the array would turn any future unchecked caller into an out of bounds read. There is a test asserting the padding entries are false.

Two tests added: one asserting baseCellIsPentagon agrees with baseCellData[].isPentagon for every base cell, so the remaining duplication cannot drift, and one for the padding. I verified the sync test actually fails when the table is perturbed rather than passing vacuously.

On the other half of #984

The issue also asks to inline _isBaseCellPentagon. I tried it and I do not think you want it.

Making it a static inline in baseCells.h measurably slows benchmarkIsValidCell. Interleaved A/B/C on the same machine, pentagonChildren_8_14_null_2, microseconds per iteration:

variant	run 1	run 2	run 3
master	621.3	625.4	620.4
this PR (table dedupe only)	621.1	619.1	620.9
dedupe + static inline	663.3	636.5	635.1

I also tried defining the table in the header as static const so each TU could constant fold it, in case the regression was from losing compile time visibility into the contents. Same ~635. So the cost tracks the inline function itself, not the array linkage.

That result makes sense in hindsight: isValidCell reaches the flags through _hasDeletedSubsequence, which already indexed the flat array directly after #968, so the hot path was not calling _isBaseCellPentagon in the first place. There was no call left to inline away, and adding the inline definition only perturbed codegen.

So this PR keeps _isBaseCellPentagon out of line and takes only the deduplication, which measures neutral. Happy to close #984 or leave it open for the array-of-structs vs struct-of-arrays question on the rest of baseCellData, whichever you prefer.

Testing

• ctest full suite: 317/317 pass
• clang-format clean on all changed files
• benchmark numbers above

Disclosure

Written with AI assistance (Claude). The benchmark numbers are measured, not estimated, and the mutation check on the new test was run. Happy to explain or revise any part of it.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coveralls]

coverage: 99.7%. remained the same — phanisaimunipalli:dedupe-pentagon-table into uber:master

[xhon-pelushi]

Built this and checked the tables mechanically rather than by eye, since "two hardcoded tables, nothing keeping them in sync" is exactly the kind of thing where a transcription slip would be invisible in review. It is a genuine no-op refactor and the new tests have teeth. Three observations below, none of them blocking.

The dedup is provably behaviour-preserving

The moved table and the old private one list the same twelve indices — 4, 14, 24, 38, 49, 58, 63, 72, 83, 97, 107, 117 — so the h3Index.c sites are unchanged by construction.

The interesting half is _isBaseCellPentagon, which switches from baseCellData[bc].isPentagon to the new array. Nothing previously forced those two to agree, so I parsed all 122 baseCellData rows out of baseCells.c and extracted the isPentagon column:

rows parsed: 122
isPentagon == 1 at base cells: [4, 14, 24, 38, 49, 58, 63, 72, 83, 97, 107, 117]
new/old table indices        : [4, 14, 24, 38, 49, 58, 63, 72, 83, 97, 107, 117]
MATCH: True

They already agreed, so no behaviour changes anywhere. Worth having on the record, because that's the one thing a reader can't confirm at a glance.

The 128-sizing argument holds and is worth keeping. H3_BC_MASK is ((uint64_t)127 << H3_BC_OFFSET), so H3_GET_BASE_CELL really can yield anything in 0..127 and _hasDeletedSubsequence(h, H3_GET_BASE_CELL(h)) is entitled to all of it. Naming that as NUM_BASE_CELL_VALUES is an improvement on the old bare 128. For the record, both present callers are bounds-safe anyway — constructCell returns E_BASE_CELL_DOMAIN before the read, and the other gets a masked 7-bit value — so the padding is belt-and-braces rather than load-bearing today. Given #1224 landed a fix for exactly this class of bug last week, keeping it seems clearly right.

The new tests fail when they should. Perturbed the table both ways:

dropped [63] = true   -> baseCellIsPentagon_matchesBaseCellData failed at testBaseCellsInternal.c:53
added   [125] = true  -> baseCellIsPentagon_paddingIsFalse      failed at testBaseCellsInternal.c:64

Full suite on the PR head: 317/317 ctest passing, testBaseCellsInternal 136 assertions.

1. The performance rationale points at paths that were already compact

The header comment says the split is so "hot paths touch one contiguous byte array instead of striding through the much larger BaseCellData structs". But the two h3Index.c sites — constructCell and _hasDeletedSubsequence — were already reading a compact 128-byte array, namely the private one being removed. For them this change is a rename.

The only site whose memory layout actually changes is _isBaseCellPentagon, and its callers are algos.c:519, five sites in localij.c, vertex.c:69, and h3Index.c:836/:1000. isValidCell is not among them — it reaches the table through _hasDeletedSubsequence, which was already on the compact array. So the function #984 names as the motivation isn't affected by the layout change at all.

2. #984 asks for a benchmark; I couldn't produce a signal

The layout difference is real on paper — sizeof(BaseCellData) is 28, so the pentagon flags are spread across 3416 bytes / 54 cache lines versus 128 bytes / 2 lines. But measuring benchmarkVertex, which does go through _isBaseCellPentagon, 15 interleaved runs per branch (µs/iteration, medians):

benchmark	master	this PR	delta
cellToVertexes	1.4220	1.4614	+2.8%
cellToVertexesPent	0.0509	0.0473	−7.1%
cellToVertexesRing	22.5597	19.1965	−14.9%
cellToVertexesRingPent	18.5569	19.9143	+7.3%

Deltas in both directions at up to ±15%, with same-branch run-to-run spread as wide as the between-branch difference. That's noise on my machine, not signal — I can't claim either an improvement or a regression. benchmarkIsValidCell was flat too, which per (1) is what you'd expect.

I'd land this on the single-source-of-truth grounds, which are solid on their own, and either drop the performance sentence from the comment or pair it with a measurement from a quiet, pinned machine. The issue does specifically say "maybe benchmark the array of structs vs struct of arrays", so it seems worth answering rather than asserting.

3. It removes the only in-tree pointer to #984, while the headline ask is still open

#984 is titled "Inline _isBaseCellPentagon", and its body is about getting inline/extern to work so isValidCell can use it; the AoS/SoA benchmark is a trailing note. This PR deletes the // TODO: https://github.com/uber/h3/issues/984 comment from h3Index.c — the only reference to #984 anywhere under src/ — while _isBaseCellPentagon is still an out-of-line call.

"Partly addresses #984" is an accurate description, but the reminder leaves with the comment. Moving the TODO onto _isBaseCellPentagon in baseCells.c, or leaving a note on the issue about what's left, would keep the trail.

What I didn't test

x86-64 Linux, GCC 13, Release only. The benchmark numbers come from a shared machine with no CPU pinning or isolation, which is why I'm reporting them as inconclusive rather than as a result — a quiet box may well show the cache-line difference that the layout suggests.