Add CMake clang-tidy integration, enable in CI, and fix all clang-tidy errors#30
Conversation
- Add CLANG_TIDY option to CMakeLists.txt that sets CMAKE_CXX_CLANG_TIDY using find_program to locate clang-tidy - Install clang-tidy in CI workflow - Pass -DCLANG_TIDY=ON for Linux CI builds Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
|
@copilot add a .clang-tidy configuration that enables warnings as errors, uses FormatStyle "file", and enables all categories of errors. |
…tStyle file Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Added |
|
@copilot you make a good point, can you enable all the sensible checks you can think of then, and not the ones that are project or company dependent?
|
…t/company-specific ones Enable: bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, hicpp, misc, modernize, performance, portability, readability. Exclude: abseil, altera, android, boost, darwin, fuchsia, google, linuxkernel, llvm, llvmlibc, mpi, objc, openmp, zircon. Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Updated in Excluded project/company/platform-specific categories: |
|
@copilot great, now run the build step you have modified to add clang tidy, look a errors it produces, and makes the necessary changes to the code to fix them. |
- Replace #define with constexpr for PRINT_HASH_SIZE - Add braces to single-statement for loops and if blocks - Add const to variables that are not modified - Replace std::endl with '\n' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace #define HSZ with constexpr size_t - Fix implicit widening of multiplication with static_cast - Add braces around single-line if body - Add const to seconds variable - Fix narrowing conversion with static_cast<double> - Replace std::endl with '\n' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace macro with constexpr variable - Add const qualifiers where appropriate - Use nullptr instead of 0 for std::time - Replace std::endl with newline character - Use static_cast instead of C-style casts - Split multi-variable declaration - Use range-based for loop - Add braces around single-line if body - Fix implicit bool conversion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace macro with constexpr - Add const qualifiers where appropriate - Use nullptr instead of 0 for std::time - Replace std::endl with '\n' - Fix implicit widening with static_cast - Split multiple variable declarations - Use auto with static_cast instead of C-style casts - Add braces around single-statement blocks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds first-class clang-tidy support to the CMake build and CI pipeline, introduces a repository .clang-tidy configuration, and updates the library header and multiple test programs to satisfy clang-tidy checks.
Changes:
- Add a
CLANG_TIDYCMake option and enable clang-tidy in Ubuntu CI builds. - Add a curated
.clang-tidyconfig and a CI step to run clang-tidy onmerklecpp.h. - Refactor/fix various test and header code patterns to address clang-tidy findings (casts, const-correctness, braces, iostream usage, etc.).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
CMakeLists.txt |
Adds CLANG_TIDY option and wires clang-tidy into CMake builds. |
.github/workflows/ci.yml |
Installs clang-tidy on Ubuntu and runs it in CI (including a header check). |
.clang-tidy |
Adds project-wide clang-tidy ruleset with warnings-as-errors. |
merklecpp.h |
Multiple clang-tidy-driven refactors and safety checks in core types/algorithms. |
test/time_large_trees.cpp |
Modernizes constants/printing/casts and timing calculations. |
test/serialise_to_file.cpp |
Updates constants/loops and file I/O style. |
test/serialisation.cpp |
Refactors constants/casts/printing; minor structural changes. |
test/retract.cpp |
Refactors constants/casts/printing; adjusts a retract invariant check. |
test/paths.cpp |
Refactors constants/casts/printing and adds explicit error branches. |
test/past_root.cpp |
Refactors constants/casts/printing and adds braces. |
test/past_paths.cpp |
Refactors constants/casts/printing; removes unused main args; adds braces. |
test/partial_serialisation.cpp |
Refactors constants/casts/printing; adds braces. |
test/flush.cpp |
Refactors constants/casts/printing; updates insertion loop. |
test/demo_tree.cpp |
Refactors constants/printing and adds braces for single-line conditionals. |
test/compare_hash_functions.cpp |
Refactors constants/printing/typedefs and improves const-correctness. |
Comments suppressed due to low confidence (1)
test/serialise_to_file.cpp:63
- The file read loop uses
while (!f.eof())and unconditionallypush_back(t)afterread(). This commonly appends an extra byte (the last value oft) when the finalread()fails. Prefer a loop that only pushes when the read succeeds (e.g.,while (f.read(&t, 1))).
while (!f.eof())
{
f.read(&t, 1);
bytes.push_back(t);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: cjen1-msft <chrisjensen@microsoft.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Move clang-tidy from global CMAKE_CXX_CLANG_TIDY to per-target CXX_CLANG_TIDY property set in add_unit_test(). This avoids running clang-tidy on third-party sources (e.g., EverCrypt) which would fail with warnings-as-errors. Also fix const on seconds variable in time_large_trees.cpp by introducing ec_seconds for the EVERCRYPT block. Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/6de1fbaf-7309-4ea8-a9fa-40d9a24c65bc Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
|
@achamayou Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
|
@copilot try again |
…rect The copilot-pull-request-reviewer flagged past_root() for only handling PATH_LEFT direction elements. Investigation shows this is intentionally correct: in a Merkle tree path, PATH_LEFT elements represent sibling subtrees added *before* the target index (part of the past state), while PATH_RIGHT elements represent subtrees added *after* (future state). Skipping PATH_RIGHT is the correct algorithm for computing past roots. Added a deterministic test that verifies past_root at every leaf index for trees of size 2-64, ensuring both PATH_LEFT and PATH_RIGHT path elements are exercised. Also changed the loop variable from copy (auto) to const reference (const auto&) for efficiency. Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/0475cd6b-501c-4961-ac1a-e2cfe1c0f89d Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Agent-Logs-Url: https://github.com/microsoft/merklecpp/sessions/0475cd6b-501c-4961-ac1a-e2cfe1c0f89d Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Ran the full build with clang-tidy in both Debug and Release modes ( |
Adds idiomatic CMake support for running clang-tidy during builds via per-target
CXX_CLANG_TIDYproperties, activates it in CI, and fixes all resulting clang-tidy errors across the codebase.CMakeLists.txt: NewCLANG_TIDYoption (defaultOFF). When enabled, usesfind_programto locateclang-tidyand sets theCXX_CLANG_TIDYproperty per-target viaadd_unit_test(), avoiding running clang-tidy on third-party sources (e.g., EverCrypt)..github/workflows/ci.yml: Installclang-tidyon Ubuntu runners; pass-DCLANG_TIDY=ONand-DCMAKE_EXPORT_COMPILE_COMMANDS=ONfor all Linux builds. Added a dedicated "Clang-Tidy Header" CI step that runsclang-tidydirectly onmerklecpp.husing the compile database, since the header-only library is not compiled as a translation unit..clang-tidy: New configuration file with curated check categories, treating all warnings as errors (WarningsAsErrors: '*'), and using the existing.clang-formatfor formatting (FormatStyle: file). Enables broadly applicable categories (bugprone-*,cert-*,clang-analyzer-*,concurrency-*,cppcoreguidelines-*,hicpp-*,misc-*,modernize-*,performance-*,portability-*,readability-*) while excluding project/company/platform-specific and overly opinionated checks.merklecpp.h: Fixed all clang-tidy findings including braces, const-correctness,usinginstead oftypedef,[[nodiscard]],noexcepton move constructors,std::movefor rvalue ref params, null-pointer guard, and more.#define→constexpr,std::endl→'\n', braces, const,static_cast, split declarations,nullptr, narrowing/widening conversions, etc.). Fixed a real bug inretract.cpp: astd::runtime_errorwas constructed but never thrown.past_root()verification: Investigated a reviewer concern aboutpast_root()only handlingPATH_LEFTdirection elements. Confirmed this is intentionally correct:PATH_LEFTelements represent sibling subtrees from the past state, whilePATH_RIGHTelements represent future state and must be skipped. Added a deterministic test verifyingpast_root()at every leaf index for trees of size 2–64.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.