Repository navigation
Clang tidy - #87
Merged
Merged
Clang tidy#87
Conversation
PierreMarchand20
force-pushed
the
clang-tidy
branch
from
September 11, 2026 10:46
b31031e to
9f8b6c0
Compare
Includes headers from the old types/ directory (split into hmatrix/ and matrix/ since). Unreferenced anywhere in the tree, CMake, or tests -- dead code that wouldn't even compile if used. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
Includes headers that no longer exist in the tree, and its CMakeLists.txt was never add_subdirectory'd -- it silently never built or ran. Found via clang-tidy run over all test/example TUs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
Mirrors the clang_format/cmake_format pattern: a `tidy` target run on demand, not part of ALL, not wired via CMAKE_CXX_CLANG_TIDY. Checks scoped to bugprone-*/performance-* in .clang-tidy; easily-swappable-parameters and random-generator-seed disabled (low-signal here: same-typed adjacent params and deterministic test seeds are intentional). Excludes wrapper_blas.hpp/wrapper_lapack.hpp, whose macro-generated bindings trigger bugprone-macro-parentheses with no real risk behind it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
rhs.m_pivots was copied, not moved (rhs is an lvalue in the function body). Also mark both noexcept, since a non-noexcept move constructor makes std::vector<Matrix<T>> fall back to copying on reallocation. Found via clang-tidy (bugprone-exception-escape / performance-noexcept-move-constructor / performance-move-constructor-init). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
TreeNode's constructors were public, so it could be built with a Derived not matching the object's actual type -- underlying()'s static_cast<Derived&> relies on that invariant. Made them private + friend Derived. Folded CRTPHelper<T> into TreeNode directly: the two-level shape tripped up clang-tidy's crtp-constructor-accessibility check, and Cluster/HMatrix already derive straight from TreeNode<Derived,...>. Verified: Cluster/HMatrix still build and link; zero crtp-constructor-accessibility findings left. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
…e reads
Two patterns flagged by clang-tidy's performance-unnecessary-value-param:
- Sink parameters (shared_ptr setters/constructor members assigned
once) were copied instead of moved: Logger::set_current_writer,
HMatrix::set_low_rank_generator/set_admissibility_condition,
ClusterTreeBuilder::set_partitioning_strategy, HMatrixTreeBuilder's
constructors/setters, RestrictedGlobalToLocalOperator's constructor,
RecompressedLowRankGenerator's constructor. Now std::move'd.
- Parameters only ever read (never mutated/stored) took an
unnecessary copy per call: Logger::log, join()'s accumulate lambda,
ClusterPartitioning::distributed_splittings, save/read_cluster_tree,
save_clustered_geometry, save_leaves_with_rank/save_levels,
vector_to_bytes. Now const&.
Also removed two dead locals ('prefix', 'sub_delimiter') noticed via
IDE diagnostics on the same files.
Verified: build-tests + build-examples clean; unnecessary-value-param
findings 68->0, unused-local-non-trivial-variable 12->0.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
std::make_unique<approximation_type>(...) was constructed and never used, triggering a clang-tidy warning when compiling the solver tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
clang-tidy's performance-avoid-endl flags std::endl for its implicit flush. Applied by hand rather than via --fix, which corrupted files when run across TUs sharing headers (caught before committing, reverted, redone safely). All 374 occurrences verified to be plain `<< (std::)endl` stream insertions, and 'endl' never appears as a substring elsewhere, so a blind textual replacement is exact. Verified: build-tests + build-examples clean; the 3 solver test files changed (HPDDM disabled in this sandbox) spot-checked with clang++ -fsyntax-only. performance-avoid-endl findings 129->0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
Copy ctor/operator=/resize computed row*col sizes safely as std::size_t, then discarded that and recomputed via raw int*int for the actual new[]/copy_n calls -- exactly where a 32-bit overflow would corrupt the allocation size. Reuse the size_t values throughout. resize()'s three-branch clone (all ending in the same `new T[...]; m_is_owning_data = true;`) is collapsed to one size_t-safe path, and now skips allocating for a zero-size buffer instead of leaving an unfreed zero-length array behind (assign()'s cleanup guard only fires for size > 0). Also removed the dead aspect_ratio() stub in partitioning.hpp (unused, non-void return, empty body). Verified: build-tests + build-examples clean; ctest passes (mpi-tagged failures are a pre-existing sandbox limitation, not a regression). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
Most of what bugprone-implicit-widening-of-multiplication-result and bugprone-narrowing-conversions flagged elsewhere was defensive-only (MPI_Reduce counts, std::setw widths, test nr=500) -- not worth casting. Kept only the two with an actual bug: - matrix/utils/output.hpp: same raw-int-product allocation-size overflow already fixed in matrix.hpp. Reused the once-computed std::size_t instead. - hmatrix_output.hpp: local_number_of_rows/cols were typed `double` for no reason, forcing back-and-forth conversions. Retyped to std::size_t. Verified: build-tests + build-examples clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ErMCMMqUWPBrTxCqzKW5VF
PierreMarchand20
force-pushed
the
clang-tidy
branch
from
September 11, 2026 10:55
9f8b6c0 to
a2ffbb7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
+ Coverage 97.19% 98.08% +0.88%
==========================================
Files 80 183 +103
Lines 5132 10004 +4872
==========================================
+ Hits 4988 9812 +4824
- Misses 144 192 +48 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Adds an opt-in
tidyCMake target (bugprone-*/performance-*, not part of the default build) and fixes what running it overinclude/,tests/, andexamples/turned up: a couple of real bugs (Matrix move ctor not movingm_pivots, int-overflow-prone allocation sizes, adouble-typo in hmatrix_output.hpp), some dead code (orphanedproto_ddm, an orphaned never-built test), and a batch of unnecessary-value-param / avoid-endl / CRTP-accessibility cleanups.Verified locally: full build-tests + build-examples clean, non-MPI ctest suite passing, plus a local HPDDM build run to cover the DDM/overlap-solver path touched by the Matrix::resize() zero-size fix.
CHANGELOG updated under Unreleased.