refactor(api)!: normalize public declarations under cdt - #120
Merged
Conversation
Move supported declarations into the cdt namespace, classify implementation helpers under detail, and document the supported API boundary. Retain line and branch coverage while disabling unreliable GCC function coverage records. BREAKING CHANGE: Global and unrelated top-level project names are removed. Downstream code must qualify supported APIs through cdt. Closes #115
acgetchell
enabled auto-merge
July 22, 2026 16:45
WalkthroughThe project’s C++ declarations are consolidated under the ChangesC++ API namespace normalization
Validation and documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Move_command_test.cpp`:
- Line 120: Replace the assertions calling the implementation-only
ergodic_moves::detail::check_move in Move_command_test.cpp with assertions
against the supported public move-command behavior and its required outcomes.
Apply the same change to all referenced assertion groups, and only introduce a
public validation API if the invariant cannot be verified through existing
public interfaces.
In `@tests/Public_api_consumer.cpp`:
- Around line 32-52: Extend the requires expression in the public API compile
contract to include propose_32_move, propose_26_move, propose_62_move, and
propose_44_move alongside propose_23_move, asserting each returns Move_result
with the same manifold and random arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79142b65-3c12-4aa6-9962-69f8f5810a06
📒 Files selected for processing (50)
README.mddocs/Doxyfiledocs/api-boundary.mddocs/ergodic-moves.mdinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Geometry.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_command.hppinclude/Move_strategy.hppinclude/Move_tracker.hppinclude/Mpfr_value.hppinclude/Periodic_3_complex.hppinclude/Periodic_3_triangulations.hppinclude/Runtime_config.hppinclude/S3Action.hppinclude/Settings.hppinclude/Torus_d.hppinclude/Triangulation_traits.hppinclude/Utilities.hppscripts/coverage.shsrc/cdt-viewer.cppsrc/cdt.cppsrc/initialize.cpptests/Apply_move_test.cpptests/Bistellar_flip_test.cpptests/CMakeLists.txttests/Ergodic_moves_3_audit_test.cpptests/Ergodic_moves_3_test.cpptests/Foliated_triangulation_test.cpptests/Function_ref_test.cpptests/Geometry_test.cpptests/Manifold_test.cpptests/Metropolis_test.cpptests/Move_always_test.cpptests/Move_command_test.cpptests/Move_tracker_test.cpptests/Public_api_consumer.cpptests/Random_benchmark.cpptests/Random_test.cpptests/Runtime_config_test.cpptests/S3Action_test.cpptests/Settings_test.cpptests/Tetrahedron_test.cpptests/Torus_test.cpptests/Utilities_test.cpptests/Vertex_test.cpp
- Add a sourceable pkgx environment for CLion and shells. - Keep project dependencies under vcpkg control. - Use the permanent Zenodo concept DOI and refresh the uv pin. - Keep public move validation independent of internal helpers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/pkgx-env.sh`:
- Line 8: Add a line-scoped ShellCheck SC2296 disable with a brief explanatory
note immediately before the intentional Zsh-specific assignment to script_file.
Keep the existing `${(%):-%x}` expression and surrounding behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9456aa88-88f2-4807-9ab9-3eeb2525510b
📒 Files selected for processing (5)
JustfileREADME.mdscripts/pkgx-env.shtests/Move_command_test.cpptests/Public_api_consumer.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/pkgx-env.sh (3)
56-65: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail fast if macOS toolchain lookup fails.
command -vonly provesxcode-selectandxcrunexist;xcode-select -porxcrun --sdk macosx --show-sdk-pathcan still fail here, leaving emptyDEVELOPER_DIR/SDKROOTvalues exported and the setup continuing with a broken environment. Check both commands and return a clear error before exporting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pkgx-env.sh` around lines 56 - 65, Update the Darwin toolchain setup block to validate the exit status and non-empty output of xcode-select -p and xcrun --sdk macosx --show-sdk-path before exporting DEVELOPER_DIR and SDKROOT. If either lookup fails, print a clear error and exit or return immediately; only export the environment variables after both values are valid.
28-29: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid clobbering caller variables in a sourced script.
repo_root,script_dir,script_file,had_allexport, andpkgx_environmentare assigned in the caller’s shell and then unset again, so any pre-existing values are lost. Use prefixed temporaries or save and restore the originals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pkgx-env.sh` around lines 28 - 29, Update the sourced-script cleanup around pkgx_environment, repo_root, script_dir, script_file, and had_allexport so it does not overwrite or unset caller-owned variables. Use uniquely prefixed temporary names, or capture each original value and restoration state before assignment and restore them before returning, while preserving the existing failure exit behavior.
36-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate pkgx eval failures.
eval "${pkgx_environment}"can hide failures inside the generated shell and still leave the script continuing with a partially applied environment. Route that path through the existing nonzero failure handling, withset +arestored before returning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/pkgx-env.sh` around lines 36 - 39, Update the pkgx environment evaluation flow around eval "${pkgx_environment}" to detect nonzero failures and route them through the script’s existing failure handling instead of continuing. Ensure set +a is restored before returning from the failure path, while preserving the current success behavior and had_allexport conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/pkgx-env.sh`:
- Around line 56-65: Update the Darwin toolchain setup block to validate the
exit status and non-empty output of xcode-select -p and xcrun --sdk macosx
--show-sdk-path before exporting DEVELOPER_DIR and SDKROOT. If either lookup
fails, print a clear error and exit or return immediately; only export the
environment variables after both values are valid.
- Around line 28-29: Update the sourced-script cleanup around pkgx_environment,
repo_root, script_dir, script_file, and had_allexport so it does not overwrite
or unset caller-owned variables. Use uniquely prefixed temporary names, or
capture each original value and restoration state before assignment and restore
them before returning, while preserving the existing failure exit behavior.
- Around line 36-39: Update the pkgx environment evaluation flow around eval
"${pkgx_environment}" to detect nonzero failures and route them through the
script’s existing failure handling instead of continuing. Ensure set +a is
restored before returning from the failure path, while preserving the current
success behavior and had_allexport conditional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 443beae4-9ac3-49d1-9f58-b75895b7d566
📒 Files selected for processing (1)
scripts/pkgx-env.sh
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move supported declarations into the cdt namespace, classify implementation helpers under detail, and document the supported API boundary. Retain line and branch coverage while disabling unreliable GCC function coverage records.
BREAKING CHANGE: Global and unrelated top-level project names are removed. Downstream code must qualify supported APIs through cdt.
Closes #115
Summary by CodeRabbit
cdtsurface, header classification, and compatibility expectations).cdtnamespace and scoped implementation/detail helpers separately.scripts/pkgx-env.shfor interactive environment setup; disabled function coverage in coverage reports; pinneduvversion.