chore: consolidate develop into main - #100
Merged
Merged
Conversation
Replace foliated_triangulations::vertices_from_cell, which only works on one cell, with manifolds::get_vertices_from_cells, which works on a container of cells. Move it to the foliated_triangulations namespace.
Nothing major, unless it trips up clang-format on GitHub Actions. Also update to vcpkgGitCommitId: 9ff4659a075d5f4f30aaca9f7c4e7f059ecc1d6d
For whatever reason, the same .clang-format produces different results on clang-format-13 vs clang-format-14. So, update to use clang-format-14 on GitHub Actions.
The bistellar_flip_really function completes the (4,4) move. Of course, it has bugs that need to be fixed, namely that it apparently leaves the Triangulation Data Structure in an invalid state, thus failing all of the checks against is_valid() which eventually leads to a std::terminate. So, this will need to be fixed. Also, update vcpkgGitCommitId: 89295c9fe22e97ca9e380a6c771ccf2ff09dca95, which incorporates the fixed version of date in vcpkg which fixes the use-of-uninitialized-value result from MSAN.
Update vcpkgGitCommitId: 5d66a98f64188bbd806b8e62e66c097d90eb3224
Update vcpkgGitCommitId: c37cc7836a0e1cfd55747be8ec472eafa8055276
While debugging bistellar_flip, make sure do_44_move returns the manifold. Fix input parameter on do_44_move to be a const reference to the manifold. Update to Boost 1.79 and qt6.3 via vcpkgGitCommitId: ecd55433fd37129109f663548f3b11204e35e630 Also proposed a pull request harupy/find-trailing-whitespace#18 to only check whitespace on changed files.
Also update README.md.
Working through the minimal example in bistellar.cpp revealed that any edge was selected due to infinite incident cells being counted. So, add a check to not count infinite cells.
The source file bistellar.cpp now contains a working minimal example of a bistellar flip. Also added a CTest to check for a successful run.
Fix documentation links and update the bistellar flip example.
Add find_vertex and find_cell helper functions to FoliatedTriangulation. These are wrappers for CGAL::Triangulation_3::is_vertex and ::is_cell, which are boolean functions with out parameters. The plan is to recast these as free functions taking an Delaunay triangulation parameter, so that they can be used in the bistellar.cpp example. That way, if they are accepted into Triangulation_3 then bistellar_flip_really becomes easy to write in terms of internal Triangulation_3 functions. Also update GitHub Actions to vcpkgGitCommitId: a106de33bbee694e3be6243718aa2a549a692832. Change README to reflect that vcpkg manifest mode is now the default, so vcpkg install --feature-flags=manifests is no longer necessary.
Yes, unified call syntax here would be nice. All tests pass. Note that hilarity (i.e. bugs) ensues if foliated_triangulations::collect_edges takes a Delaunay triangulation by value instead of constant reference. Update to vcpkgGitCommitId: 47cbed88514c0e48fdb72ddc29545f35136f5461
Catch2 has a number of issues with ASAN, compile times, etc. doctest is much faster and simpler. There are a couple of tests that need to be rewritten, and have been commented out.
Update vcpkgGitCommitId: 2e781b513f486618909d9f78185c0c7ebb54624c
Mostly to try to see if we can get less output from ASAN.
Update vcpkgGitCommitId: 840f701d83d5019aa5033c9d9d08a4cc0d0ebdce which should fix spdlog.
Also take advantage of std::numbers in C++20.
Should rename functions to be consistent.
Geometry3 -> Geometry_3 FoliatedTriangulation3 -> Foliated_Triangulation_3 etc. New function incident_cells_from_edge to encapsulate all the logic behind finding locations for bistellar flips based on the number of incident cells (ie. == 4). Updated to vcpkgGitCommitId: c379c64ff9ec1b8c6100ebff8e81ecdcb49b330c
Only need a reference to the triangulation, the edge to be flipped, and the top and bottom vertices. Update vcpkgGitCommitId: 1323f4b83d6a91e415e9ce06a45c9d0a6473aaf2 The function still needs to be debugged, of course.
Turn on doctest fast assertions. Fix references to Catch in documentation and CI. Use doctest test suites. Update CI to use vcpkgGitCommitId: 9e8da9bd8aa16fb268ac68f731c44fdef5fec1ab
PVS Studio identified possible use of an uninitialized value obtained by a function returning a std::optional<T>.
# ---------------------------------------------------------- # Header - (type): Brief description # ---------------------------------------------------------- # * feature A new feature # * fix A bug fix # * docs Changes to documentation only # * style Style/format changes (whitespace, etc.) # * refactor Changes not related to a bug or feature # * performance Changes that affects performance # * test Changes that add/modify/correct tests # * build Changes to build system (configs, etc.) # * ci Changes to CI pipeline/workflow # ---------------------------------------------------------- # ---------------------------------------------------------- # Body - More detailed description, if necessary # ---------------------------------------------------------- # * Motivation behind changes, more detail into how # functionality might be affected, etc. # ---------------------------------------------------------- # ---------------------------------------------------------- # Footer - Associated issues, PRs, etc. # ---------------------------------------------------------- # * Ex: Resolves Issue #207, see PR #15, etc. # ----------------------------------------------------------
fmt::print doesn't like CGAL point << output anymore, so wrap it with a point_to_str that converts it. Fix Metropolis to use SystemError.what() which is a const string for reporting via spdlog::trace which uses fmt which requires const string arguments. Similar conversions using Gmpzf.to_double to make spdlg::debug happy. Update vcpkgGitCommitId: 23cc58477e200bb54c293ad76f3ce438dbabc86c
Nice catch via sonarcloud. Also remove unused includes.
Found a few more places to fix fmt. Simplified try_32_move. Removed diagnostic code that was throwing exceptions. That info gets printed out before the runs already. Updated vcpkgGitCommitId: 5fb8f6c24ec13a36db460a555b24b20be03bd73c
Except the usual transparent functors 'less<>' issue which is unfixable given docopt.cpp's current implementation.
Consistent formatting
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 66
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/fast-build.sh (1)
3-13: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBehavioral asymmetry vs.
fast-build.bat: no VCPKG_ROOT validation/fallback here.
fast-build.batruns the same--check/pinned-fallback dance asbuild.batbefore building. This script just documents the precondition ("make sure $VCPKG_ROOT names the pinned checkout") and trusts the caller, with no validation and no fallback to the repository-pinned vcpkg. If a stale/incorrectVCPKG_ROOTis exported, the Unix fast path silently builds against it while the Windows fast path would reject/replace it. Worth confirming this divergence is intentional (e.g., "fast" is meant to skip the check for speed) rather than an oversight.🤖 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/fast-build.sh` around lines 3 - 13, Update the fast-build script’s setup before the cmake commands to match fast-build.bat: validate VCPKG_ROOT with the existing check and fall back to the repository-pinned vcpkg when it is missing or incorrect. Keep the current repository-root resolution and fast build targets unchanged.
🤖 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 @.clang-format:
- Line 206: Optionally update the Standard setting in .clang-format from c++20
to Latest so formatting tracks the repository’s C++23 constructs; otherwise
leave the existing c++20 value unchanged.
In @.clang-tidy:
- Line 4: Update the WarningsAsErrors configuration in .clang-tidy from the
unusual '0' value to the idiomatic empty string '', preserving the behavior of
treating no warnings as errors.
- Line 237: Correct the readability-suspicious-call-argument.Abbreviations entry
by separating the dist and dst mappings with a semicolon, preserving both
dist=distance and dst=distance abbreviations.
- Line 16: Update the HeaderFilterRegex configuration to anchor matching at the
repository root and match headers under the include, src, or tests directories;
preserve the existing intended directory scope while correcting the leading-path
pattern.
In @.cmake-format.yaml:
- Around line 2-10: Remove the placeholder `additional_commands.foo`
configuration, including its `flags` and `kwargs` entries for `BAR`, `BAZ`,
`DEPENDS`, `HEADERS`, and `SOURCES`, unless the project has a real custom
command requiring this definition.
In @.coderabbit.yaml:
- Line 27: Update the path filter in the review configuration from the singular
test directory to the tests directory so the doctest-specific instructions apply
to files matching tests/**/*.cpp. Leave the surrounding review configuration
unchanged.
In @.github/CONTRIBUTING.md:
- Line 57: Correct the git log example in the contributing documentation by
closing the single-quoted --format argument before the pipe to sort -u. Preserve
the intended command behavior of sorting unique author names.
In @.github/ISSUE_TEMPLATE/bug_report.md:
- Around line 26-35: Update the Desktop and Smartphone sections in the bug
report template to collect project-relevant details instead of browser, device,
and mobile OS information, including compiler and version, operating system, and
build preset. Remove the irrelevant fields while retaining clear prompts and
example values for the replacement fields.
In @.github/workflows/asan.yml:
- Around line 54-72: Update the sanitizer workflow steps so failures remain
observable while preserving execution of subsequent checks: in
.github/workflows/asan.yml lines 54-72, .github/workflows/lsan.yml lines 54-72,
and .github/workflows/msan.yml lines 54-72, assign ids to the ctest and
sanitizer-run steps and add a final validation step that fails the job when any
captured outcome is failure. Apply the same outcome-check pattern to all three
files, covering every step currently using continue-on-error.
In @.github/workflows/ci.yml:
- Around line 44-48: Update the “Windows MSVC” matrix entry to use the existing
compiler_package key instead of the unused pkgx key, preserving its empty value
and aligning it with the other matrix entries.
In @.github/workflows/clang-format-check.yml:
- Around line 4-14: Add a concurrency block after the "on" section in
.github/workflows/clang-format-check.yml lines 4-14,
.github/workflows/codeql.yml lines 15-21, .github/workflows/cpp-linter.yml lines
4-11, and .github/workflows/doxygen.yml lines 4-8. In each file, use the
existing workflow/ref group pattern and enable cancellation of in-progress runs
to prevent redundant overlapping executions.
In @.github/workflows/codecov-upload.yml:
- Around line 62-74: Update the coverage generation step’s find path to use the
CDT_unit_tests.dir target directory defined by tests/CMakeLists.txt instead of
CDT_test.dir, while preserving the existing object-file processing and gcov
flow.
In @.github/workflows/doxygen.yml:
- Around line 10-15: Move the contents: write permission from the workflow-level
permissions block into the deploy job definition, keeping it scoped only to
deploy and preserving its ability to push gh-pages.
In @.github/workflows/whitespace.yml:
- Around line 13-16: Add a concurrency block to the workflow configuration near
the existing top-level permissions/jobs sections, matching the concurrency group
and cancellation behavior used by tsan.yml and valgrind.yml so superseded runs
on the same ref are cancelled.
In @.gitignore:
- Line 37: Correct the filename pattern in .gitignore from CMakeSEttings.json to
CMakeSettings.json so it matches Visual Studio’s generated file on
case-sensitive filesystems.
In `@cmake/Cache.cmake`:
- Around line 27-29: Update the fallback message in the CACHE_OPTION handling
branch to use STATUS instead of WARNING when the optional cache tool is
unavailable, preserving the existing message and build behavior.
In `@cmake/Sanitizers.cmake`:
- Around line 18-26: Update the Valgrind branch in enable_sanitizers to use the
`${project_name}` target for both target_compile_options and
target_link_libraries instead of hardcoding project_options, matching the target
used by the other branches and preserving reuse with different project targets.
In `@CMakeLists.txt`:
- Line 72: Update the CGAL_DATA_DIR assignment to dereference CMAKE_BINARY_DIR
so it contains the actual build Data path rather than a literal string. Also
confirm that configure-time set(ENV{...}) behavior is appropriate for the
intended build and run steps before retaining this mechanism.
- Around line 41-43: Update the ENABLE_BUILD_WITH_TIME_TRACE block to use
target_compile_options for the project_options target instead of
add_compile_definitions, ensuring -ftime-trace is applied as a compiler option
to that target rather than treated as a preprocessor definition.
In `@include/Apply_move.hpp`:
- Around line 35-50: Remove the noexcept specifier from the apply_move function
template so exceptions thrown by the forwarded move callable can propagate
normally. Leave the existing std::expected result handling and logging
unchanged.
In `@include/Ergodic_moves_3.hpp`:
- Line 951: Guard the diagnostic fmt::print in find_pivot_edge with the file’s
existing `#ifndef` NDEBUG pattern, matching neighboring trace statements so
release builds remain silent.
- Around line 338-339: Run clang-format on include/Ergodic_moves_3.hpp, focusing
on the signatures for is_62_movable, bistellar_flip, and check_move, then apply
the formatter’s whitespace and alignment changes across the file so the
formatting check passes.
- Around line 489-560: Consolidate the repeated edge-validation and
incident-cell circulator logic from incident_cells_from_edge,
get_incident_cells, and find_pivot_edge into one shared helper that collects
finite incident cells. Preserve the existing invalid-edge behavior and each
caller’s required return semantics, including the bistellar size check in
find_bistellar_flip_location.
- Around line 696-734: Move the n1_idx through n8_idx computations below the
neighbor validation in the affected move function, so n_1 through n_8 are
confirmed non-null and not scheduled for deletion before any ->index call.
Preserve the existing neighbor checks and index values once validation succeeds.
In `@include/Foliated_triangulation.hpp`:
- Around line 1196-1199: Update the get_delaunay() method to return
m_triangulation directly, removing the redundant std::cref wrapper while
preserving its Delaunay const& return type.
In `@include/Formatters.hpp`:
- Line 34: Reformat the format method signature to match the project’s
clang-format style, using east-const for the CGAL::Point_3<Kernel> reference
parameter. Keep the method behavior unchanged.
In `@include/Manifold.hpp`:
- Around line 145-149: Update the get_triangulation() accessor to return
m_triangulation directly instead of wrapping it with std::cref, preserving the
existing const-reference return type and noexcept behavior.
- Around line 152-155: Update Manifold::get_delaunay() to preserve the reference
returned by get_triangulation().get_delaunay() by using decltype(auto) or an
equivalent const-reference return type, avoiding a copied triangulation and
preserving chained-use behavior.
In `@include/Metropolis.hpp`:
- Line 389: Update the command initialization around initialize(t_manifold) to
avoid value_or’s eager MoveCommand(t_manifold) construction. Use an explicit
optional-success branch that moves or reuses the initialized command, and
construct MoveCommand(t_manifold) only in the failure path.
In `@include/Move_always.hpp`:
- Around line 102-125: Refactor the move-selection logic around move_choice and
ManifoldType::dimension so the dimension check is performed once, then dispatch
move_choice through a switch or the existing move_tracker::as_move mapping.
Preserve the current enqueue behavior for choices 0 through 4 and the
surrounding MoveCommand::as_move_function conventions.
- Line 12: Implement the missing initialization step for the Move_always
strategy, matching the initialization behavior used by Metropolis. Wire the
strategy into the existing initialization flow and include the associated
checkpoint/reporting setup, then remove or update the stale “Fix initialization”
bug annotation.
In `@include/Move_strategy.hpp`:
- Around line 13-14: Update the include guard in Move_strategy.hpp from
INCLUDE_MOVE_ALGORITHM_HPP_ to a name matching the file, such as
INCLUDE_MOVE_STRATEGY_HPP_, and apply the same identifier consistently to the
corresponding `#define` and closing `#endif` guard comment.
In `@include/Move_tracker.hpp`:
- Around line 141-144: Add the direct <numeric> standard-library include to
Move_tracker.hpp so the std::accumulate call in total() is declared without
relying on transitive includes.
In `@include/Periodic_3_triangulations.hpp`:
- Around line 68-77: Update the nested loops in the timeslice generation block
to use signed index types matching the int variables timeslices and
points_per_timeslice, and give the outer and inner loops distinct index names.
Apply the corresponding renamed index in the debug output and
point-generation/printing accesses while preserving the existing iteration
behavior.
In `@include/Settings.hpp`:
- Around line 28-32: Update the preprocessor condition surrounding the
Int_precision alias to use the portable defined(__linux__) check instead of
testing __linux directly, preserving the existing Linux and non-Linux type
selections.
In `@include/Torus_d.hpp`:
- Around line 34-46: Update make_d_cube so the populated point vector reaches
the caller: either accept t_points by reference and preserve the existing
caller-owned output contract, or return the populated vector and adjust callers
accordingly. Ensure points_on_cube_grid_d writes to the same vector the caller
observes.
In `@include/Utilities.hpp`:
- Around line 161-176: Update the write_file template to accept its
triangulation parameter as a const reference instead of by value, preserving the
existing serialization behavior and allowing both lvalue triangulations and
temporaries such as t_universe.get_delaunay() to bind correctly.
- Around line 264-297: Remove constexpr from generate_random_int,
generate_random_real, and generate_probability in Utilities.hpp, leaving their
runtime behavior and forwarding logic unchanged.
In `@Justfile`:
- Around line 89-156: The _format-check and _format-fix recipes duplicate
clang-format and clang-format-diff discovery; extract that shared resolution
logic into a private helper script or recipe, then have both recipes reuse its
resolved paths while preserving the existing version validation, pkgx fallback,
and missing-tool errors.
In `@README.md`:
- Around line 337-341: Update the Doxygen configuration path in the README
command to use the portable forward-slash form docs/Doxyfile, matching the
executable command and avoiding Unix shell interpretation issues.
- Line 475: Remove the unused Markdown reference-link definitions, including the
[gcc] definition and the additional definitions at the referenced ranges, unless
their links are intentionally restored and referenced elsewhere in README.md.
- Around line 278-322: Replace the affected Markdown code fences in README.md,
including the blocks around the shown usage output and the additional referenced
sections, from triple tildes to triple backticks for both opening and closing
delimiters. Preserve all fenced content unchanged.
- Line 398: Update the documented command in README.md to reference the
repository’s actual scripts/cppcheck.sh name after changing into scripts,
replacing cppcheck-build.sh while preserving the surrounding command flow.
In `@scripts/asan.sh`:
- Around line 6-8: Replace the fragile relative cd in the ASAN build script with
robust script-directory and repository-root resolution, then perform rm -rf
build and the cmake invocation from the resolved repository root. Preserve the
existing build directory cleanup and sanitizer configuration while ensuring the
script works regardless of the caller’s current working directory.
In `@scripts/bootstrap-vcpkg.bat`:
- Around line 59-102: Ensure the batch script is checked out with CRLF line
endings by adding or updating the repository’s attributes configuration to
explicitly mark .bat files as text with CRLF normalization. Preserve the
existing CALL :VALIDATE_CACHED_VCPKG flow and do not alter the script logic.
In `@scripts/bootstrap-vcpkg.sh`:
- Around line 36-40: Update the cached-checkout reuse branch in the bootstrap
script around the `${vcpkg_dir}/vcpkg` executable check to validate the binary’s
integrity before accepting it. Apply the same release pin and SHA-256
verification used by the Windows cache path, and only print “Using vcpkg” and
exit when that verification succeeds; otherwise continue through the normal
bootstrap flow.
In `@scripts/build.bat`:
- Around line 13-29: The reference Windows build flow is missing the cache reset
used before reconfiguration. Add the equivalent removal of CMakeCache.txt and
CMakeFiles for the reference build directory in the batch script before the
cmake --preset reference invocation, while preserving the existing VCPKG_ROOT
selection and error handling.
In `@scripts/clang-tidy.sh`:
- Around line 6-8: Replace the fragile working-directory assumption in the
clang-tidy script around the build setup commands: resolve the repository root
from the script’s own location, then run the cleanup and CMake commands from
that directory. Ensure rm -rf build/ can only target the repository build
directory regardless of the caller’s current directory.
In `@scripts/codecov.sh`:
- Around line 6-8: Replace the fragile relative cd in the build setup with
directory resolution based on the scripts/codecov.sh location, then enter the
repository root before removing and recreating build. Preserve the existing rm
-rf build and mkdir build behavior.
- Line 17: Replace the legacy remote bash uploader in the codecov script with
the repository’s existing codecov/codecov-action workflow or the supported
Codecov CLI, reusing the established configuration and preserving coverage.info
upload behavior and failure handling.
In `@scripts/cppcheck.sh`:
- Around line 6-7: Replace the fragile relative `cd ..` in the script’s
working-directory setup with a location-independent approach based on the
script’s own directory, while preserving the subsequent build cleanup performed
by `rm -rf build`.
In `@scripts/debug.sh`:
- Around line 6-7: Replace the fragile relative `cd ..` in the debug script with
a repository-root resolution based on the script’s own location, then remove
build/ using that resolved root. Preserve the existing cleanup behavior while
making execution independent of the caller’s working directory.
In `@scripts/fast-build.bat`:
- Around line 13-24: Convert scripts/fast-build.bat to Windows CRLF line endings
and preserve the VCPKG_ROOT fallback flow around the VCPKG_READY label. Ensure
cmd.exe reliably parses the CALL, GOTO VCPKG_READY, and label directives without
changing their behavior.
In `@scripts/gcov.sh`:
- Around line 6-7: Replace the fragile `cd ..` in the gcov script with a
location-independent project-root resolution, then run the cleanup against that
resolved root so the script works regardless of the caller’s working directory.
Preserve the existing removal of the build directory.
- Around line 13-17: Fix the gcov invocation in the report-generation loop: use
the directory containing the coverage data for -o and pass the actual source or
object target instead of the literal x. Update the find iteration in the script
around pushd to safely handle paths, quote variable expansions, and ensure
directory changes fail or are restored reliably with matching popd.
In `@scripts/lsan.sh`:
- Line 12: Replace the ambiguous --s launcher option with the explicit -s short
flag in scripts/lsan.sh at lines 12-12, scripts/tsan.sh at lines 12-12, and
src/optimize-initialize.py at lines 66-68. Preserve the remaining initialize
arguments and command behavior unchanged.
In `@scripts/msan.sh`:
- Around line 6-8: Harden both analysis scripts by adding strict mode
immediately after each shebang and guarding the directory change before
destructive cleanup: in scripts/msan.sh lines 6-8, update cd .. to cd .. || exit
before rm -rf build; in scripts/pvs-studio.sh lines 10-11, apply the same
strict-mode and guarded-cd changes before rm -rf build/.
In `@src/cdt.cpp`:
- Line 23: Update the comments near the CLI help message and argument handling
to remove stale references to docopt and describe Boost.Program_options
accurately. Change only the comments associated with the help text and the
arguments passed into the parser, preserving the existing code behavior.
- Around line 68-99: Ensure the command-line options bound to alpha, k, and
lambda are validated before any later use, rather than allowing omitted values
to remain uninitialized. Update the argument-validation flow around po::notify
and the existing simplices/timeslices checks to reject missing parameters, or
mark these options required while processing --help and --version before calling
po::notify.
In `@src/optimize-initialize.py`:
- Around line 124-131: Remove the unconditional traceback.print_exc() from the
finally block in the exception-handling flow. Invoke traceback.print_exc() only
within handlers that catch an actual exception, including the TypeError/KeyError
handler, while preserving the NoMoreSuggestionsAvailable success message and
avoiding traceback output on clean completion.
- Around line 80-91: Replace each regex pattern string passed to re.findall in
the parsing branches around result[1], result[2], result[0], and Timeslice with
a raw string literal, preserving the existing \d+ pattern and parsing behavior.
In `@src/test.py`:
- Line 42: Update the model.evaluate call to pass y_test alongside x_test
instead of y_train, preserving the existing verbose setting.
In `@tests/Bistellar_flip_test.cpp`:
- Around line 30-105: Give the file-local helpers create_test_triangulation,
verify_triangulation_validity, and verify_neighbor_relationships internal
linkage by placing them in an anonymous namespace or marking them static,
without changing their behavior.
In `@tests/Settings_test.cpp`:
- Around line 23-31: Update the integer-type assertion in the “The integer type
is queried” test to mirror the __linux conditional used by Int_precision in
Settings.hpp: compare against int on Linux and std::int_fast32_t elsewhere, and
adjust the THEN description so it reflects the platform-specific expected type.
In `@tests/Torus_test.cpp`:
- Around line 11-42: Add the direct <boost/type_index.hpp> include in
tests/Torus_test.cpp alongside the existing headers so the boost::typeindex
usage in the “The type is queried” test is self-contained.
In `@tests/Utilities_test.cpp`:
- Around line 129-155: The round-trip test’s filename must remain consistent
across its writing, reading, and deleting subcases. Update the test around
write_file and utilities::read_file to use one deterministic fixed temporary
path, or pass the same explicit path through the path-based overload, instead of
relying on make_filename(manifold) or internally recomputed timestamps.
---
Outside diff comments:
In `@scripts/fast-build.sh`:
- Around line 3-13: Update the fast-build script’s setup before the cmake
commands to match fast-build.bat: validate VCPKG_ROOT with the existing check
and fall back to the repository-pinned vcpkg when it is missing or incorrect.
Keep the current repository-root resolution and fast build targets 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: 8f33cc9b-7d81-4396-82c5-d64bd543dfa3
⛔ Files ignored due to path filters (11)
config/travisci_rsa.pubis excluded by!**/*.pubdocs/images/26.pngis excluded by!**/*.pngdocs/images/44.pngis excluded by!**/*.pngdocs/images/S3-7-27528-I1-R1.pngis excluded by!**/*.pngdocs/images/t8-v68-s298.pngis excluded by!**/*.pngsrc/date/test/tz_test/tzdata2015e.txt.zipis excluded by!**/*.zipsrc/date/test/tz_test/tzdata2015f.txt.zipis excluded by!**/*.zipsrc/date/test/tz_test/tzdata2016c.txt.zipis excluded by!**/*.zipsrc/date/test/tz_test/tzdata2016d.txt.zipis excluded by!**/*.zipsrc/date/test/tz_test/tzdata2016e.txt.zipis excluded by!**/*.zipsrc/date/test/tz_test/tzdata2016f.txt.zipis excluded by!**/*.zip
📒 Files selected for processing (282)
.appveyor.yml.clang-format.clang-tidy.cmake-format.yaml.codecov.yml.coderabbit.yaml.github/CONTRIBUTING.md.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/dependabot.yml.github/issue_template.md.github/workflows/asan.yml.github/workflows/ci.yml.github/workflows/clang-format-check.yml.github/workflows/codecov-upload.yml.github/workflows/codeql.yml.github/workflows/cpp-linter.yml.github/workflows/cppcheck.yml.github/workflows/doxygen.yml.github/workflows/lsan.yml.github/workflows/msan.yml.github/workflows/tsan.yml.github/workflows/valgrind.yml.github/workflows/whitespace.yml.gitignore.pinact.yaml.travis.yml.yamllint.ymlCMakeLists.txtCMakePresets.jsonJustfileLICENSE.mdREADME.mdbuild.shclang-tidy.shcmake/.clang-tidycmake/Cache.cmakecmake/CompilerWarnings.cmakecmake/PreventInSourceBuilds.cmakecmake/Sanitizers.cmakecmake/StandardProjectSettings.cmakecmake/StaticAnalyzers.cmakeconfig/publish-doxygenconfig/travisci_rsa.enccppcheck-build.shdocs/Doxyfiledocs/images/26.epsinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Formatters.hppinclude/Function_ref.hppinclude/Geometry.hppinclude/Manifold.hppinclude/Measurements.hppinclude/Metropolis.hppinclude/MoveAlgorithm.hppinclude/MoveAlways.hppinclude/MoveManager.hppinclude/Move_always.hppinclude/Move_command.hppinclude/Move_strategy.hppinclude/Move_tracker.hppinclude/Periodic_3_complex.hppinclude/Periodic_3_triangulations.hppinclude/S3Action.hppinclude/S3ErgodicMoves.hppinclude/S3Triangulation.hppinclude/Settings.hppinclude/SimplicialManifold.hppinclude/Simulation.hppinclude/Sphere_d.hppinclude/Torus_d.hppinclude/Triangulation_traits.hppinclude/Utilities.hppscan.shscripts/asan.shscripts/bootstrap-vcpkg.batscripts/bootstrap-vcpkg.shscripts/build.batscripts/build.shscripts/clang-tidy.shscripts/codecov.shscripts/cppcheck.shscripts/debug.shscripts/fast-build.batscripts/fast-build.shscripts/gcov.shscripts/iwyu.shscripts/lsan.shscripts/msan.shscripts/pkgx-build.shscripts/pvs-studio.shscripts/slurm.shscripts/tsan.shslurm.shsonar-project.propertiessrc/CMakeLists.txtsrc/bistellar-flip.cppsrc/cdt-gv.cppsrc/cdt-opt.cppsrc/cdt-viewer.cppsrc/cdt.cppsrc/date/.gitignoresrc/date/.gitreposrc/date/CMakeLists.txtsrc/date/LICENSE.txtsrc/date/README.mdsrc/date/compile_fail.shsrc/date/include/date/chrono_io.hsrc/date/include/date/date.hsrc/date/include/date/ios.hsrc/date/include/date/ios.mmsrc/date/include/date/islamic.hsrc/date/include/date/iso_week.hsrc/date/include/date/julian.hsrc/date/include/date/ptz.hsrc/date/include/date/tz.hsrc/date/include/date/tz_private.hsrc/date/src/tz.cppsrc/date/test/clock_cast_test/custom_clock.pass.cppsrc/date/test/clock_cast_test/deprecated.pass.cppsrc/date/test/clock_cast_test/noncastable.pass.cppsrc/date/test/clock_cast_test/normal_clocks.pass.cppsrc/date/test/clock_cast_test/to_sys_return_int.fail.cppsrc/date/test/clock_cast_test/to_sys_return_reference.fail.cppsrc/date/test/clock_cast_test/to_sys_return_utc_time.fail.cppsrc/date/test/date_test/day.pass.cppsrc/date/test/date_test/daypday.fail.cppsrc/date/test/date_test/daysmday.fail.cppsrc/date/test/date_test/daysmweekday.fail.cppsrc/date/test/date_test/detail/decimal_format_seconds.pass.cppsrc/date/test/date_test/detail/make_precision.pass.cppsrc/date/test/date_test/detail/static_pow10.pass.cppsrc/date/test/date_test/detail/width.pass.cppsrc/date/test/date_test/durations.pass.cppsrc/date/test/date_test/format/century.pass.cppsrc/date/test/date_test/format/misc.pass.cppsrc/date/test/date_test/format/range.pass.cppsrc/date/test/date_test/format/two_dight_year.pass.cppsrc/date/test/date_test/last.pass.cppsrc/date/test/date_test/make_time.pass.cppsrc/date/test/date_test/month.pass.cppsrc/date/test/date_test/month_day.pass.cppsrc/date/test/date_test/month_day_last.pass.cppsrc/date/test/date_test/month_weekday.pass.cppsrc/date/test/date_test/month_weekday_last.pass.cppsrc/date/test/date_test/month_weekday_last_less.fail.cppsrc/date/test/date_test/month_weekday_less.fail.cppsrc/date/test/date_test/monthpmonth.fail.cppsrc/date/test/date_test/months_m_year_month.fail.cppsrc/date/test/date_test/months_m_year_month_day.fail.cppsrc/date/test/date_test/monthsmmonth.fail.cppsrc/date/test/date_test/op_div_day_day.fail.cppsrc/date/test/date_test/op_div_int_month.fail.cppsrc/date/test/date_test/op_div_int_year.fail.cppsrc/date/test/date_test/op_div_last_last.fail.cppsrc/date/test/date_test/op_div_month_day.pass.cppsrc/date/test/date_test/op_div_month_day_last.pass.cppsrc/date/test/date_test/op_div_month_day_month_day.fail.cppsrc/date/test/date_test/op_div_month_month.fail.cppsrc/date/test/date_test/op_div_month_weekday.pass.cppsrc/date/test/date_test/op_div_month_weekday_last.pass.cppsrc/date/test/date_test/op_div_month_year.fail.cppsrc/date/test/date_test/op_div_survey.pass.cppsrc/date/test/date_test/op_div_weekday_indexed_weekday_indexed.fail.cppsrc/date/test/date_test/op_div_weekday_last_weekday_last.fail.cppsrc/date/test/date_test/op_div_year_month.pass.cppsrc/date/test/date_test/op_div_year_month_day.pass.cppsrc/date/test/date_test/op_div_year_month_day_last.pass.cppsrc/date/test/date_test/op_div_year_month_weekday.pass.cppsrc/date/test/date_test/op_div_year_month_weekday_last.pass.cppsrc/date/test/date_test/op_div_year_month_year_month.fail.cppsrc/date/test/date_test/op_div_year_year.fail.cppsrc/date/test/date_test/parse.pass.cppsrc/date/test/date_test/sizeof.pass.cppsrc/date/test/date_test/time_of_day_hours.pass.cppsrc/date/test/date_test/time_of_day_microfortnights.pass.cppsrc/date/test/date_test/time_of_day_milliseconds.pass.cppsrc/date/test/date_test/time_of_day_minutes.pass.cppsrc/date/test/date_test/time_of_day_nanoseconds.pass.cppsrc/date/test/date_test/time_of_day_seconds.pass.cppsrc/date/test/date_test/weekday.pass.cppsrc/date/test/date_test/weekday_indexed.pass.cppsrc/date/test/date_test/weekday_last.pass.cppsrc/date/test/date_test/weekday_lessthan.fail.cppsrc/date/test/date_test/weekday_sum.fail.cppsrc/date/test/date_test/year.pass.cppsrc/date/test/date_test/year_month.pass.cppsrc/date/test/date_test/year_month_day.pass.cppsrc/date/test/date_test/year_month_day_last.pass.cppsrc/date/test/date_test/year_month_day_m_year_month_day.fail.cppsrc/date/test/date_test/year_month_day_p_year_month_day.fail.cppsrc/date/test/date_test/year_month_p_year_month.fail.cppsrc/date/test/date_test/year_month_weekday.pass.cppsrc/date/test/date_test/year_month_weekday_last.pass.cppsrc/date/test/date_test/year_p_year.fail.cppsrc/date/test/date_test/years_m_year.fail.cppsrc/date/test/date_test/years_m_year_month.fail.cppsrc/date/test/date_test/years_m_year_month_day.fail.cppsrc/date/test/iso_week/last.pass.cppsrc/date/test/iso_week/lastweek_weekday.pass.cppsrc/date/test/iso_week/op_div_survey.pass.cppsrc/date/test/iso_week/weekday.pass.cppsrc/date/test/iso_week/weekday_lessthan.fail.cppsrc/date/test/iso_week/weekday_sum.fail.cppsrc/date/test/iso_week/weeknum.pass.cppsrc/date/test/iso_week/weeknum_p_weeknum.fail.cppsrc/date/test/iso_week/weeknum_weekday.pass.cppsrc/date/test/iso_week/year.pass.cppsrc/date/test/iso_week/year_lastweek.pass.cppsrc/date/test/iso_week/year_lastweek_weekday.pass.cppsrc/date/test/iso_week/year_p_year.fail.cppsrc/date/test/iso_week/year_weeknum.pass.cppsrc/date/test/iso_week/year_weeknum_weekday.pass.cppsrc/date/test/iso_week/years_m_year.fail.cppsrc/date/test/just.pass.cppsrc/date/test/testitsrc/date/test/tz_test/OffsetZone.pass.cppsrc/date/test/tz_test/README.mdsrc/date/test/tz_test/validate.cppsrc/date/test/tz_test/zone.pass.cppsrc/date/test/tz_test/zoned_time.pass.cppsrc/date/test_fail.shsrc/docopt/.gitignoresrc/docopt/.gitreposrc/docopt/.travis.ymlsrc/docopt/CMakeLists.txtsrc/docopt/LICENSE-Boost-1.0src/docopt/LICENSE-MITsrc/docopt/README.rstsrc/docopt/docopt-config.cmakesrc/docopt/docopt.cppsrc/docopt/docopt.hsrc/docopt/docopt_private.hsrc/docopt/docopt_util.hsrc/docopt/docopt_value.hsrc/docopt/examples/naval_fate.cppsrc/docopt/main.cppsrc/docopt/run_testcase.cppsrc/docopt/run_tests.pysrc/docopt/testcases.docoptsrc/initialize.cppsrc/optimize-initialize.pysrc/test.pytest/CMakeLists.txttest/FunctionRefTest.cpptest/MeasurementsTest.cpptest/MoveAlgorithmsTest.cpptest/MoveManagerTest.cpptest/S3ActionTest.cpptest/S3ErgodicMovesTest.cpptest/S3TriangulationTest.cpptest/SimplicialManifoldTest.cpptest/SimulationTest.cpptest/SphereTest.cpptest/TetrahedronTest.cpptest/TorusTest.cpptest/UtilitiesTest.cpptest/VertexTest.cpptest/catch.hpptest/main.cpptests/Apply_move_test.cpptests/Bistellar_flip_test.cpptests/CMakeLists.txttests/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/S3Action_test.cpptests/Settings_test.cpptests/Tetrahedron_test.cpptests/Torus_test.cpptests/Utilities_test.cpptests/Vertex_test.cpptests/main.cppvcpkg.json
💤 Files with no reviewable changes (31)
- build.sh
- cppcheck-build.sh
- .github/issue_template.md
- src/date/.gitignore
- .appveyor.yml
- src/date/LICENSE.txt
- clang-tidy.sh
- scan.sh
- src/date/README.md
- include/Function_ref.hpp
- sonar-project.properties
- src/date/include/date/ios.mm
- config/publish-doxygen
- src/cdt-gv.cpp
- src/date/include/date/ios.h
- src/date/include/date/chrono_io.h
- .travis.yml
- include/MoveAlgorithm.hpp
- src/date/CMakeLists.txt
- include/MoveAlways.hpp
- include/Simulation.hpp
- src/date/compile_fail.sh
- slurm.sh
- include/Measurements.hpp
- include/MoveManager.hpp
- src/date/.gitrepo
- include/Sphere_d.hpp
- src/date/include/date/iso_week.h
- include/SimplicialManifold.hpp
- include/S3ErgodicMoves.hpp
- include/S3Triangulation.hpp
- enforce causal 2-to-3 proposals and implement failure-atomic 4-to-4 moves with checked CGAL primitives - preserve manifold settings, validate runtime bounds, and prevent move counters from carrying across runs - make sanitizer, CodeQL, Codecov, and cross-platform CI failures actionable - add locked Python 3.12 quality tooling and retire PVS-Studio
Correct the immutable action pin used to install Just and align the reported C++ files with clang-format 18.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
Justfile (1)
164-180: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftCheck committed changes, not only the working tree.
Line 168 compares the worktree with
HEAD; in a clean checkout, includingjust ci, that diff is empty. The formatter gate then skips every tracked C/C++ change. Diff against an explicit PR/base revision (with the base fetched) or validate tracked files directly.🤖 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 `@Justfile` around lines 164 - 180, The _format-check recipe currently compares HEAD with its worktree, so committed changes are skipped in clean checkouts and CI. Update its tracked-file validation to compare against the explicit PR/base revision, ensuring that base revision is available, or otherwise format-check all tracked C/C++ files directly; preserve the existing untracked-file validation.include/Metropolis.hpp (1)
234-239: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCorrect the
(6,2)action delta.A
(6,2)move removes four combined(3,1)/(1,3)cells and no(2,2)cells, but this calculates the opposite cell-type delta.A2therefore uses the wrong acceptance probability.Proposed fix
case move_tracker::move_type::SIX_TWO: newS3Action = - S3_bulk_action(m_geometry.N1_TL - 2, m_geometry.N3_31_13, - m_geometry.N3_22 - 4, m_Alpha, m_K, m_Lambda); + S3_bulk_action(m_geometry.N1_TL - 2, + m_geometry.N3_31_13 - 4, + m_geometry.N3_22, m_Alpha, m_K, m_Lambda);🤖 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 `@include/Metropolis.hpp` around lines 234 - 239, Update the SIX_TWO branch in the move_tracker action calculation to reflect removal of four combined (3,1)/(1,3) cells and no (2,2) cells: decrement the N3_31_13 argument by 4 while leaving N3_22 unchanged, and preserve the existing N1_TL decrement.include/Move_always.hpp (1)
112-120: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not expose silent 4D no-op strategies. Both generic strategy implementations only execute 3D moves, despite the exported 4D aliases. A 4D run either performs no moves or applies 3D acceptance logic.
include/Move_always.hpp#L112-L120: implement 4D move dispatch, or reject unsupported dimensions before the run starts.include/Metropolis.hpp#L293-L303: implement 4D move generation andCalculateA2<4>, or constrain this specialization to 3D.🤖 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 `@include/Move_always.hpp` around lines 112 - 120, Prevent exported 4D strategies from silently doing nothing or using 3D logic. In include/Move_always.hpp lines 112-120, add 4D move dispatch or reject unsupported dimensions before execution; in include/Metropolis.hpp lines 293-303, add 4D move generation with CalculateA2<4> or constrain the specialization to 3D. Ensure both affected strategy implementations expose only supported behavior.include/Utilities.hpp (1)
215-296: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove
noexceptfrom the RNG helpers.die_roll,generate_random,make_random_generator,generate_random_int,generate_random_timeslice,generate_random_real, andgenerate_probabilityall seedpcg64fromstd::random_device; if that initialization throws,noexceptturns it intostd::terminateinstead of an exception.🤖 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 `@include/Utilities.hpp` around lines 215 - 296, Remove the noexcept specifier from the RNG helper functions die_roll, generate_random, make_random_generator, generate_random_int, generate_random_timeslice, generate_random_real, and generate_probability so exceptions from random_device or pcg64 initialization can propagate normally; leave their existing behavior and signatures otherwise unchanged.tests/Ergodic_moves_3_test.cpp (1)
558-563: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKnown-invalid post-flip triangulation is only
WARN-ed here.This scenario builds the
Delaunayfrom raw points without setting vertexinfo()timevalues, then downgrades the validity check toWARNwith a FIXME ("neighbor of c has not c as neighbor"). Note thattests/Bistellar_flip_test.cpp(Lines 119–123) exercises the samebistellar_flipbutREQUIREs success and assertsis_valid()/tds().is_valid()— the difference is that test seeds vertex timevalues viacreate_test_triangulation. So the invariant gap tracked by this FIXME is real and currently uncovered by a hard assertion.Want me to open an issue to track the post-flip neighbor-reciprocity failure so this
WARNcan eventually become aCHECK?🤖 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 `@tests/Ergodic_moves_3_test.cpp` around lines 558 - 563, Replace the WARN-only validity check in the flipped_triangulation branch with a hard assertion, and ensure the raw-point Delaunay setup initializes vertex info() timevalues consistently with create_test_triangulation before bistellar_flip runs. Preserve the existing post-flip validity and neighbor-reciprocity expectations.Source: Path instructions
.clang-tidy (1)
144-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the clang-tidy option spellings in
.clang-tidy.NamePrefixSuffixSilenceDissimilarityTreshold→NamePrefixSuffixSilenceDissimilarityThreshold,CharTypdefsToIgnore→CharTypedefsToIgnore(both entries), andFowardIt→ForwardItat lines 144, 213, 270, and 347.🤖 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 @.clang-tidy at line 144, Correct the misspelled clang-tidy option keys in .clang-tidy: rename NamePrefixSuffixSilenceDissimilarityTreshold to NamePrefixSuffixSilenceDissimilarityThreshold, CharTypdefsToIgnore to CharTypedefsToIgnore for both entries, and FowardIt to ForwardIt, without changing their values or surrounding configuration.
🤖 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 @.github/workflows/lsan.yml:
- Line 62: Replace the unsupported long sanitizer option with the
CTest-supported short option: update ./initialize --s to ./initialize -s at
.github/workflows/lsan.yml:62-62 and .github/workflows/msan.yml:56-56, and
update ./cdt --s to ./cdt -s at .github/workflows/lsan.yml:70-70 and
.github/workflows/msan.yml:64-64.
In @.github/workflows/valgrind.yml:
- Around line 58-75: Replace the ambiguous `--s` arguments with `-s` in the
Valgrind commands for `initialize` and `cdt`. Leave the existing options and
Valgrind configuration unchanged.
In `@scripts/codecov.sh`:
- Around line 14-18: Add -DCMAKE_BUILD_TYPE=Debug to the cmake configure command
in scripts/codecov.sh, alongside the existing ENABLE_COVERAGE option, so
single-config generators use Debug while preserving the current build and test
commands.
In `@scripts/fast-build.sh`:
- Around line 19-20: After exporting VCPKG_ROOT and running bootstrap-vcpkg.sh
in the fast-build flow, clear the existing out/build/reference cache before
configuration. Mirror the cache-reset behavior used by scripts/build.bat so
stale toolchain and dependency paths cannot be reused.
---
Outside diff comments:
In @.clang-tidy:
- Line 144: Correct the misspelled clang-tidy option keys in .clang-tidy: rename
NamePrefixSuffixSilenceDissimilarityTreshold to
NamePrefixSuffixSilenceDissimilarityThreshold, CharTypdefsToIgnore to
CharTypedefsToIgnore for both entries, and FowardIt to ForwardIt, without
changing their values or surrounding configuration.
In `@include/Metropolis.hpp`:
- Around line 234-239: Update the SIX_TWO branch in the move_tracker action
calculation to reflect removal of four combined (3,1)/(1,3) cells and no (2,2)
cells: decrement the N3_31_13 argument by 4 while leaving N3_22 unchanged, and
preserve the existing N1_TL decrement.
In `@include/Move_always.hpp`:
- Around line 112-120: Prevent exported 4D strategies from silently doing
nothing or using 3D logic. In include/Move_always.hpp lines 112-120, add 4D move
dispatch or reject unsupported dimensions before execution; in
include/Metropolis.hpp lines 293-303, add 4D move generation with CalculateA2<4>
or constrain the specialization to 3D. Ensure both affected strategy
implementations expose only supported behavior.
In `@include/Utilities.hpp`:
- Around line 215-296: Remove the noexcept specifier from the RNG helper
functions die_roll, generate_random, make_random_generator, generate_random_int,
generate_random_timeslice, generate_random_real, and generate_probability so
exceptions from random_device or pcg64 initialization can propagate normally;
leave their existing behavior and signatures otherwise unchanged.
In `@Justfile`:
- Around line 164-180: The _format-check recipe currently compares HEAD with its
worktree, so committed changes are skipped in clean checkouts and CI. Update its
tracked-file validation to compare against the explicit PR/base revision,
ensuring that base revision is available, or otherwise format-check all tracked
C/C++ files directly; preserve the existing untracked-file validation.
In `@tests/Ergodic_moves_3_test.cpp`:
- Around line 558-563: Replace the WARN-only validity check in the
flipped_triangulation branch with a hard assertion, and ensure the raw-point
Delaunay setup initializes vertex info() timevalues consistently with
create_test_triangulation before bistellar_flip runs. Preserve the existing
post-flip validity and neighbor-reciprocity expectations.
🪄 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: c2b6a066-85ce-406f-8715-a26fb5b9d420
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
.clang-tidy.cmake-format.yaml.coderabbit.yaml.gitattributes.github/CONTRIBUTING.md.github/ISSUE_TEMPLATE/bug_report.md.github/ISSUE_TEMPLATE/feature_request.md.github/actions/setup-just/action.yml.github/workflows/asan.yml.github/workflows/ci.yml.github/workflows/clang-format-check.yml.github/workflows/codecov-upload.yml.github/workflows/codeql.yml.github/workflows/cpp-linter.yml.github/workflows/doxygen.yml.github/workflows/lsan.yml.github/workflows/msan.yml.github/workflows/tsan.yml.github/workflows/valgrind.yml.github/workflows/whitespace.yml.gitignore.python-versionCMakeLists.txtCMakePresets.jsonJustfileREADME.mdcmake/Cache.cmakecmake/Coverage.cmakecmake/Sanitizers.cmakecmake/StandardProjectSettings.cmakecmake/Valgrind.cmakeinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Formatters.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_strategy.hppinclude/Move_tracker.hppinclude/Periodic_3_triangulations.hppinclude/Settings.hppinclude/Torus_d.hppinclude/Utilities.hpppyproject.tomlscripts/asan.shscripts/bootstrap-vcpkg.shscripts/build.batscripts/clang-tidy.shscripts/codecov.shscripts/cppcheck.shscripts/debug.shscripts/fast-build.shscripts/gcov.shscripts/iwyu.shscripts/lsan.shscripts/msan.shscripts/slurm.shscripts/tsan.shsrc/cdt.cppsrc/optimize-initialize.pysrc/test.pytests/Apply_move_test.cpptests/Bistellar_flip_test.cpptests/Ergodic_moves_3_test.cpptests/Function_ref_test.cpptests/Metropolis_test.cpptests/Move_always_test.cpptests/Move_command_test.cpptests/Settings_test.cpptests/Torus_test.cpptests/Utilities_test.cppty.toml
💤 Files with no reviewable changes (1)
- .cmake-format.yaml
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
- make rejected moves failure-atomic and implement the causal 6→2 retriangulation - correct 6→2 Metropolis action bookkeeping and add cdt --no-output - consolidate LLVM 22 formatting, clang-tidy, sanitizer, and quality workflows - preserve compatible CMake and vcpkg caches across incremental builds - remove cdt-opt, Valgrind integration, and unsupported 4D scaffolding - document the maintenance-mode release workflow BREAKING CHANGE: Remove the cdt-opt executable and unsupported 4D manifold, geometry, move, and cell-type APIs.
- allow clean builds to proceed when no CMake cache exists - reuse pinned vcpkg checkouts containing generated artifacts - install the pinned zizmor version before repository quality checks
Remove the cppcheck workflow, CMake preset, local runner, CodeRabbit configuration, and documentation references.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/Move_command.hpp (2)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
std::cref— same pattern already flagged and fixed elsewhere in this PR.
get_const_results()returnsManifoldType const&, soreturn std::cref(m_manifold);builds an unnecessaryreference_wrapperthat immediately converts back. This is the identical anti-pattern already fixed ininclude/Foliated_triangulation.hpp'sget_delaunay()(per a previous review comment);return m_manifold;is simpler and equivalent here too.🧹 Simplify
auto get_const_results() const -> ManifoldType const& - { return std::cref(m_manifold); } // get_const_results + { return m_manifold; } // get_const_results🤖 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 `@include/Move_command.hpp` around lines 74 - 78, Update Move_command.hpp’s get_const_results() to return m_manifold directly instead of wrapping it with std::cref, preserving the declared ManifoldType const& return type.
85-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
get_succeeded()is missing the trailing return type its siblings have, so it returns by value instead of by reference.
get_attempted()andget_failed()both declare-> Counter const&, butget_succeeded()omits the trailing return type. With plainauto(no&), the deduced return type isCounter(a copy of the wholeMoveTracker), notCounter const&. Behaviorally harmless sinceMoveTrackeris cheap to copy, but it's an inconsistent public API surface for three otherwise-parallel accessors.🔧 Align the signature with its siblings
/** * \brief Successful moves by MoveCommand */ - [[nodiscard]] auto get_succeeded() const + [[nodiscard]] auto get_succeeded() const -> Counter const& { return m_succeeded; } // get_succeeded🤖 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 `@include/Move_command.hpp` around lines 85 - 101, Update MoveCommand::get_succeeded() to declare the same const-reference return type as get_attempted() and get_failed(), preserving the parallel accessor API and avoiding a returned Counter copy.include/Utilities.hpp (1)
220-231: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
generate_random(anddie_roll) reseed a fresh RNG fromstd::random_deviceon every single call — this is a hot-path perf trap.
generate_random, and by extensiongenerate_random_int,generate_random_real,generate_probability, anddie_roll, each construct a brand-newpcg_extras::seed_seq_from<std::random_device>andpcg64generator per invocation.std::random_devicetypically hits a syscall (/dev/urandom/getrandom()), so this is orders of magnitude slower than drawing from an already-seeded PRNG.make_random_generator()right below already shows the correct pattern (build once, reuse forstd::shuffle), butgenerate_randomdoesn't follow it.This matters because
generate_random_intfeedsmove_tracker::generate_random_move_3(), called once per simplex per pass in the Metropolis/MoveAlways main loops (include/Metropolis.hpp,include/Move_always.hpp), andgenerate_probability()is called once pertry_move(). For large triangulations run over many passes, this reseed-per-draw overhead compounds into a real bottleneck.⚡ Reuse a seeded generator instead of reseeding per call
template <typename NumberType, class Distribution> [[nodiscard]] auto generate_random(NumberType t_min_value, NumberType t_max_value) { - pcg_extras::seed_seq_from<std::random_device> seed_source; - // Make a random number generator - pcg64 generator(seed_source); + thread_local pcg64 generator{pcg_extras::seed_seq_from<std::random_device>{}}; Distribution distribution(t_min_value, t_max_value); return distribution(generator); } // generate_random()The same fix pattern applies to
die_roll().Also applies to: 246-264
🤖 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 `@include/Utilities.hpp` around lines 220 - 231, Update generate_random and die_roll to reuse a generator that is seeded once rather than constructing pcg_extras::seed_seq_from<std::random_device> and pcg64 on every invocation. Preserve the existing random distributions and return behavior, and ensure generate_random_int, generate_random_real, generate_probability, and die_roll all draw from the reused generator.
🤖 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 @.github/workflows/_sanitizer.yml:
- Around line 45-92: Add a reasonable timeout-minutes setting to the sanitizer
job definition so ASAN, LSAN, MSAN, and TSAN runs are terminated promptly if
they hang. Keep the existing sanitizer steps and configuration unchanged.
In `@include/Ergodic_moves_3.hpp`:
- Around line 577-582: Update the `(6,2)` move-selection logic around
`try_62_move` to iterate through all movable vertex candidates, continuing when
a candidate is rejected instead of returning failure immediately. Apply the
triangulation swap and return `t_manifold` only for the first accepted move;
preserve the existing failure behavior after every candidate has been attempted.
In `@include/Metropolis.hpp`:
- Around line 443-453: Remove the intermediate value copy in the checkpoint
block and pass the result of command.get_results() directly to
utilities::write_file. Keep the existing checkpoint and m_write_files conditions
unchanged, matching the equivalent Move_always.hpp flow while preserving
read-only access.
In `@include/Move_tracker.hpp`:
- Around line 102-111: Add a non-const move_type overload to MoveTracker
alongside the existing const operator[](move_type const) overload, returning a
mutable reference to the corresponding moves entry via as_integer(move).
Preserve the const overload for read-only MoveTracker instances so enum-based
access supports both reads and writes.
In `@scripts/clang-tidy.sh`:
- Around line 11-16: Update the LLVM version lookup in scripts/clang-tidy.sh so
it does not invoke host just before the pkgx fallback in the
CDT_CLANG_TIDY_ACTIVE branch can run. Resolve llvm_version without requiring
installed just, or move/bootstrap the just invocation through pkgx first, while
preserving the existing clang-tidy provisioning behavior.
- Around line 32-34: Update the CMake configure invocation in the clang-tidy
script to pass the cache variable VCPKG_INSTALLED_DIR instead of
_VCPKG_INSTALLED_DIR, using the existing clang_tidy_vcpkg_installed_dir value so
cache refresh logic selects the dedicated installed tree.
In `@scripts/prepare-vcpkg.sh`:
- Around line 7-16: Update the environment-clearing section of prepare-vcpkg.sh
to also unset CC and CXX before the Darwin-specific compiler assignments,
ensuring inherited compiler selectors cannot override the reference toolchain.
In `@src/initialize.cpp`:
- Around line 151-154: Update the invalid_argument message in the dimensions
validation branch of initialize so it accurately describes that unsupported
dimension values other than 3 are rejected, including values below 3, rather
than claiming only values greater than 3 are invalid.
---
Outside diff comments:
In `@include/Move_command.hpp`:
- Around line 74-78: Update Move_command.hpp’s get_const_results() to return
m_manifold directly instead of wrapping it with std::cref, preserving the
declared ManifoldType const& return type.
- Around line 85-101: Update MoveCommand::get_succeeded() to declare the same
const-reference return type as get_attempted() and get_failed(), preserving the
parallel accessor API and avoiding a returned Counter copy.
In `@include/Utilities.hpp`:
- Around line 220-231: Update generate_random and die_roll to reuse a generator
that is seeded once rather than constructing
pcg_extras::seed_seq_from<std::random_device> and pcg64 on every invocation.
Preserve the existing random distributions and return behavior, and ensure
generate_random_int, generate_random_real, generate_probability, and die_roll
all draw from the reused generator.
🪄 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: 3ac19b00-ce6b-43e9-8edf-20acb0b2c605
📒 Files selected for processing (51)
.clang-tidy.github/CONTRIBUTING.md.github/workflows/_sanitizer.yml.github/workflows/asan.yml.github/workflows/ci.yml.github/workflows/lsan.yml.github/workflows/msan.yml.github/workflows/tsan.ymlCMakeLists.txtCMakePresets.jsonJustfileREADME.mdcmake/RunCdtNoOutputTest.cmakeinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Formatters.hppinclude/Geometry.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_always.hppinclude/Move_command.hppinclude/Move_tracker.hppinclude/Utilities.hppscripts/build.batscripts/build.shscripts/clang-tidy.shscripts/codecov.shscripts/fast-build.batscripts/fast-build.shscripts/pkgx-build.shscripts/prepare-vcpkg.shscripts/sanitizer.shscripts/slurm.shsrc/CMakeLists.txtsrc/cdt-opt.cppsrc/cdt.cppsrc/initialize.cpptests/Bistellar_flip_test.cpptests/CMakeLists.txttests/Ergodic_moves_3_test.cpptests/Foliated_triangulation_test.cpptests/Manifold_test.cpptests/Metropolis_test.cpptests/Move_always_test.cpptests/Move_command_test.cpptests/Move_tracker_test.cpptests/Settings_test.cpptests/Tetrahedron_test.cpptests/Utilities_test.cpptests/Vertex_test.cpp
💤 Files with no reviewable changes (4)
- scripts/slurm.sh
- include/Geometry.hpp
- tests/Move_always_test.cpp
- src/cdt-opt.cpp
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
acgetchell
enabled auto-merge (squash)
July 19, 2026 20:31
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 (1)
scripts/prepare-vcpkg.sh (1)
68-73: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTreat missing vcpkg cache entries as stale state
prepare_cmake_cache()only invalidates when a value is present and different, so an incompleteCMakeCache.txtcan keep an outdated vcpkg setup alive. Compare the cache entries unconditionally here, and mirror the same check inscripts/build.batandscripts/fast-build.bat.🤖 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/prepare-vcpkg.sh` around lines 68 - 73, Update the cache validation in prepare_cmake_cache() to invalidate whenever cached_toolchain or cached_installed_dir differs from its expected value, including when either entry is missing. Apply the same unconditional comparisons to the corresponding vcpkg cache checks in scripts/build.bat and scripts/fast-build.bat, preserving the existing stale-cache invalidation behavior.
🤖 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/prepare-vcpkg.sh`:
- Around line 68-73: Update the cache validation in prepare_cmake_cache() to
invalidate whenever cached_toolchain or cached_installed_dir differs from its
expected value, including when either entry is missing. Apply the same
unconditional comparisons to the corresponding vcpkg cache checks in
scripts/build.bat and scripts/fast-build.bat, preserving the existing
stale-cache invalidation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 081c22ba-d458-495d-ba19-2bac9e2a3846
📒 Files selected for processing (8)
.coderabbit.yaml.github/workflows/ci.ymlCMakePresets.jsonJustfileREADME.mdcmake/StaticAnalyzers.cmakescripts/bootstrap-vcpkg.shscripts/prepare-vcpkg.sh
💤 Files with no reviewable changes (3)
- cmake/StaticAnalyzers.cmake
- CMakePresets.json
- .coderabbit.yaml
- Try every movable vertex before rejecting a 6-2 proposal. - Preserve reference semantics for move results and counters. - Bound sanitizer jobs and make clang-tidy provisioning reproducible. - Clarify 3D-only initialization and remove HDF5 from the roadmap.
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 (1)
tests/Move_tracker_test.cpp (1)
87-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the
as_movefallback path.This scenario covers only valid inputs
0–4. Add explicit cases such as-1and5, or define and test rejection behavior, because the current implementation silently maps all other integers tomove_type::FOUR_FOUR. This is based on the suppliedinclude/Move_tracker.hppimplementation. As per path instructions,tests/**/*.cppreviews must ensure comprehensive test coverage and proper doctest practices.🤖 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 `@tests/Move_tracker_test.cpp` around lines 87 - 103, Extend the “Integer to move type conversion” scenario around as_move to cover out-of-range inputs, including -1 and 5. Assert the current fallback result move_type::FOUR_FOUR for each invalid value, or update the assertions if as_move is changed to reject invalid inputs; retain the existing valid mappings for 0–4.Source: Path instructions
🤖 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 `@tests/Move_tracker_test.cpp`:
- Around line 87-103: Extend the “Integer to move type conversion” scenario
around as_move to cover out-of-range inputs, including -1 and 5. Assert the
current fallback result move_type::FOUR_FOUR for each invalid value, or update
the assertions if as_move is changed to reject invalid inputs; retain the
existing valid mappings for 0–4.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1beba4ca-dd5b-4408-be92-0e658c7e41aa
📒 Files selected for processing (12)
.github/workflows/_sanitizer.ymlREADME.mdinclude/Ergodic_moves_3.hppinclude/Metropolis.hppinclude/Move_command.hppinclude/Move_tracker.hppscripts/clang-tidy.shscripts/prepare-vcpkg.shsrc/CMakeLists.txtsrc/initialize.cpptests/Move_command_test.cpptests/Move_tracker_test.cpp
💤 Files with no reviewable changes (1)
- README.md
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 (4)
include/Move_command.hpp (2)
171-175: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude successful moves in the post-execution summary.
print_successful()is defined at Lines 217-229, butexecute()prints only attempted and failed counts. Addprint_successful()alongside the other summaries, or remove the helper if successful-count reporting is intentionally handled elsewhere.🤖 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 `@include/Move_command.hpp` around lines 171 - 175, Update the post-execution debug summary in execute() to call print_successful() alongside print_attempts() and print_errors(), ensuring successful moves are included in the reported counts.
37-40: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDrain or remove the retry queue.
Both failure paths append to
m_moves_to_retry, butexecute()only drainsm_moves. The private retry queue is never retried, exposed, or cleared; because the same command is reused across passes ininclude/Metropolis.hppLines 393-459, failed entries accumulate indefinitely. This also does not implement the PR objective of retrying movable vertices for 6-2 proposals.Implement a bounded retry policy that consumes and clears the queue, or remove the queue and its retry semantics.
Also applies to: 158-166
🤖 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 `@include/Move_command.hpp` around lines 37 - 40, Update Move_command’s retry handling so m_moves_to_retry is either consumed and cleared by execute() under a bounded retry policy, or removed together with all retry semantics. Ensure failed entries do not accumulate across reused command executions and that movable vertices for 6-2 proposals are actually retried.tests/Move_tracker_test.cpp (1)
118-119: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCheck each counter, not only the aggregate.
Line 119 only proves that the sum is zero; it would still pass if nonzero counters cancelled out. Iterate over
moves_view()and assert every element is zero.Suggested assertion
THEN("Each element is zero-initialized.") - { REQUIRE_EQ(tracked_moves.total(), 0); } + { + REQUIRE_EQ(tracked_moves.total(), 0); + for (auto const value : tracked_moves.moves_view()) + { + CHECK_EQ(value, 0); + } + }As per path instructions,
tests/**/*.cpprequires comprehensive test coverage and proper doctest organization.🤖 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 `@tests/Move_tracker_test.cpp` around lines 118 - 119, Update the test case “Each element is zero-initialized” to iterate through the collection returned by moves_view() and assert every counter is zero individually, rather than checking only tracked_moves.total(). Preserve the existing doctest structure and initialization coverage.Source: Path instructions
tests/Move_command_test.cpp (1)
47-50: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winVerify the entire manifold is unchanged after a failed move.
The failure branch checks only
result.simplices(). A failed topology operation could leave the same simplex count while changing connectivity or metadata, so this helper would miss a violation of the atomic-discard contract ininclude/Move_command.hppLines 66-68. Compare the full manifold state, or all relevant invariants, withbeforewhen the move fails.🤖 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 `@tests/Move_command_test.cpp` around lines 47 - 50, Update the failed-move branch in the test helper around result.simplices() to compare the complete manifold state against before, not just the simplex count. Include connectivity, metadata, and all relevant invariants needed to verify the atomic-discard contract while preserving the existing success-branch checks.
🤖 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 `@include/Move_command.hpp`:
- Around line 171-175: Update the post-execution debug summary in execute() to
call print_successful() alongside print_attempts() and print_errors(), ensuring
successful moves are included in the reported counts.
- Around line 37-40: Update Move_command’s retry handling so m_moves_to_retry is
either consumed and cleared by execute() under a bounded retry policy, or
removed together with all retry semantics. Ensure failed entries do not
accumulate across reused command executions and that movable vertices for 6-2
proposals are actually retried.
In `@tests/Move_command_test.cpp`:
- Around line 47-50: Update the failed-move branch in the test helper around
result.simplices() to compare the complete manifold state against before, not
just the simplex count. Include connectivity, metadata, and all relevant
invariants needed to verify the atomic-discard contract while preserving the
existing success-branch checks.
In `@tests/Move_tracker_test.cpp`:
- Around line 118-119: Update the test case “Each element is zero-initialized”
to iterate through the collection returned by moves_view() and assert every
counter is zero individually, rather than checking only tracked_moves.total().
Preserve the existing doctest structure and initialization coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8489374c-ce1b-4e93-82b2-49bcd8a14b54
📒 Files selected for processing (12)
.github/workflows/_sanitizer.ymlREADME.mdinclude/Ergodic_moves_3.hppinclude/Metropolis.hppinclude/Move_command.hppinclude/Move_tracker.hppscripts/clang-tidy.shscripts/prepare-vcpkg.shsrc/CMakeLists.txtsrc/initialize.cpptests/Move_command_test.cpptests/Move_tracker_test.cpp
💤 Files with no reviewable changes (1)
- README.md
- invalidate incomplete CMake/vcpkg configuration state on Unix and Windows - report successful queued moves and remove unused retry bookkeeping
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 (2)
tests/Move_command_test.cpp (1)
100-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew state-preservation checks on failure are thorough.
The addition of
get_delaunay()equality,vertex_states,cell_states, andmanifold_countscomparisons in the failure branch provides excellent coverage for the rejection invariant. The new "Rejected moves preserve manifold state" scenario (lines 429-457) complements this well with a minimal manifold that forces rejection.One minor note: the parameter named
moveon line 102 shadowsstd::movedue tousing namespace std;on line 24. Not a bug sincestd::moveisn't used in this function, but renaming tomove_typeort_movewould avoid potential confusion during future edits.♻️ Suggested rename
- void check_single_move_outcome(MoveCommand<Manifold_3> const& command, - Manifold_3 const& before, - move_tracker::move_type const move, - Int_precision const cell_delta) + void check_single_move_outcome(MoveCommand<Manifold_3> const& command, + Manifold_3 const& before, + move_tracker::move_type const t_move, + Int_precision const cell_delta) { - auto const attempted = command.get_attempted()[move]; - auto const succeeded = command.get_succeeded()[move]; - auto const failed = command.get_failed()[move]; + auto const attempted = command.get_attempted()[t_move]; + auto const succeeded = command.get_succeeded()[t_move]; + auto const failed = command.get_failed()[t_move]; CHECK_EQ(attempted, 1); CHECK_EQ(succeeded + failed, 1); auto const& result = command.get_const_results(); CHECK(result.is_valid()); if (succeeded == 1) { CHECK_EQ(result.simplices(), before.simplices() + cell_delta); - CHECK(ergodic_moves::check_move(before, result, move)); + CHECK(ergodic_moves::check_move(before, result, t_move)); }🤖 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 `@tests/Move_command_test.cpp` around lines 100 - 125, Rename the move parameter in check_single_move_outcome from move to a non-conflicting name such as move_type or t_move, and update its uses when indexing get_attempted(), get_succeeded(), get_failed(), and calling ergodic_moves::check_move.include/Move_command.hpp (1)
195-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEliminate duplication across print methods.
print_attempts,print_successful, andprint_errorsshare near-identical format strings differing only in the adjective (attempted/successful/failed) and the counter source. Extracting a common helper eliminates ~30 lines of duplicated formatting logic and ensures any future format changes apply uniformly.♻️ Proposed refactor
+ /** + * \brief Print move counts with a label + * \param label The adjective describing the move counts + * \param counter The counter to print + */ + void print_counter(std::string_view label, Counter const& counter) const + { + fmt::print( + "There were {} {} (2,3) moves and {} {} (3,2) moves and {} " + "{} (2,6) moves and {} {} (6,2) moves and {} {} (4,4) moves.\n", + counter.two_three_moves(), label, + counter.three_two_moves(), label, + counter.two_six_moves(), label, + counter.six_two_moves(), label, + counter.four_four_moves(), label); + } /** * \brief Print attempted moves */ void print_attempts() const - { - fmt::print( - "There were {} attempted (2,3) moves and {} attempted (3,2) moves " - "and {} " - "attempted (2,6) moves and {} attempted (6,2) moves and {} attempted " - "(4,4) " - "moves.\n", - m_attempted.two_three_moves(), m_attempted.three_two_moves(), - m_attempted.two_six_moves(), m_attempted.six_two_moves(), - m_attempted.four_four_moves()); - } + { print_counter("attempted", m_attempted); } /** * \brief Print successful moves */ void print_successful() const - { - fmt::print( - "There were {} successful (2,3) moves and {} successful (3,2) moves " - "and {} " - "successful (2,6) moves and {} successful (6,2) moves and {} " - "successful " - "(4,4) " - "moves.\n", - m_succeeded.two_three_moves(), m_succeeded.three_two_moves(), - m_succeeded.two_six_moves(), m_succeeded.six_two_moves(), - m_succeeded.four_four_moves()); - } + { print_counter("successful", m_succeeded); } /** * \brief Print move errors */ void print_errors() const { if (std::all_of(m_failed.moves_view().begin(), m_failed.moves_view().end(), [](auto const& value) { return value == 0; })) { fmt::print("There were no failed moves.\n"); } else - { - fmt::print( - "There were {} failed (2,3) moves and {} failed (3,2) moves and {} " - "failed (2,6) moves and {} failed (6,2) moves and {} failed (4,4) " - "moves.\n", - m_failed.two_three_moves(), m_failed.three_two_moves(), - m_failed.two_six_moves(), m_failed.six_two_moves(), - m_failed.four_four_moves()); - } + { print_counter("failed", m_failed); } }🤖 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 `@include/Move_command.hpp` around lines 195 - 245, Extract the shared move-count formatting from print_attempts, print_successful, and print_errors into a private helper that accepts the adjective and counter source, then have each print method delegate to it. Preserve print_errors’ special no-failed-moves message while using the helper for nonzero failures, and keep all existing move categories and output wording unchanged.
🤖 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 `@include/Move_command.hpp`:
- Around line 195-245: Extract the shared move-count formatting from
print_attempts, print_successful, and print_errors into a private helper that
accepts the adjective and counter source, then have each print method delegate
to it. Preserve print_errors’ special no-failed-moves message while using the
helper for nonzero failures, and keep all existing move categories and output
wording unchanged.
In `@tests/Move_command_test.cpp`:
- Around line 100-125: Rename the move parameter in check_single_move_outcome
from move to a non-conflicting name such as move_type or t_move, and update its
uses when indexing get_attempted(), get_succeeded(), get_failed(), and calling
ergodic_moves::check_move.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a86321f5-3360-40d5-a7e5-ffd39e7630f6
📒 Files selected for processing (6)
include/Move_command.hppscripts/build.batscripts/fast-build.batscripts/prepare-vcpkg.shtests/Move_command_test.cpptests/Move_tracker_test.cpp
This was referenced Jul 20, 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
2 participants
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.
Consolidate the current development history into main and make main the sole CI pull-request target.
Summary by CodeRabbit
just-based maintenance workflow (build/check/fix, clang-tidy, sanitizers).initializeandcdtCLIs with improved help/version handling and--no-outputmode.cdt-viewerfor rendering saved triangulations.