docs: document compile-time limits - #5
Conversation
|
Thanks for the PR, the documentation itself looks good. Before merging, I'd like to make this a little more robust against the exact maintenance risk you mentioned in the PR description: these values can become stale if the constants change later. Could you please add a small automated consistency check that verifies the documented limits still match the public constants? It doesn't need to be anything heavyweight. A small test or script integrated into the existing test/CI flow would be enough. Ideally it should cover:
Also, please add the defining header for each constant to the README table so users can easily find the authoritative declaration. That way this section becomes more than a snapshot of the current values — CI will prevent the documentation from silently drifting out of sync with the API. Once that's in, I'll take another look. Thanks! |
Onwcan
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I'm marking this as changes requested so it's clear that the items in my previous comment are blockers before merge.
In addition to those points, please also adjust the README section hierarchy. Compile-time limits is currently a ### heading under the ## Build section, but these limits are public API/design constraints rather than build configuration. Please make it a top-level README section (## Compile-time limits) or place it under an equivalent API/design section.
So before the next review, please address:
the automated consistency check for the three documented constants,
the defining header/source location in the README table,
the README heading hierarchy for the compile-time limits section.
Once those are updated, ping me and I'll review the new revision. Thanks!
|
Addressed all requested follow-up in a98fc5f:
Validation: debug build succeeded; all 228 tests passed, including readme_limits; scripts/format.sh --check passed (36 files). Please take another look when convenient. |
|
CI is green as well. Nice work, and thanks for following through on the review feedback. |
What this changes, and why
Documents the three public compile-time capacity limits and makes explicit that they are currently fixed rather than CMake-configurable.
Evidence
The documented values match
kMaxPathKnots,kMaxWaypoints, andkMaxObstaclesin the public headers.git diff --checkpasses; CMake tests could not run because CMake is unavailable on this host.Risk
Documentation-only change; stale values would be visible by comparison with the public constants.
Confirm before requesting review:
bash scripts/format.sh --checkpasses — CMake/tooling unavailable on this hostCHANGELOG.mdupdated underUnreleased, if behaviour or API changed — N/AFixes #4