Skip to content

Expose clang-tidy and clang-format as CMake targets#2506

Draft
nathanwbrei wants to merge 4 commits into
mainfrom
nbrei_local_clang_tidy_format
Draft

Expose clang-tidy and clang-format as CMake targets#2506
nathanwbrei wants to merge 4 commits into
mainfrom
nbrei_local_clang_tidy_format

Conversation

@nathanwbrei
Copy link
Copy Markdown
Contributor

Briefly, what does this PR introduce?

This PR introduces some new CMake targets for running clang-tidy and clang-format locally. This allows users to validate that their PR passes the style checks proactively and interactively, without having to wait for CI.

# Run clang-tidy over the entire codebase and apply, but don't commit, fixes
cmake --build build --target tidy

# Run clang-tidy over the entire codebase and return an error if changes are still needed
cmake --build build --target check-tidy     

# Run clang-format over the entire codebase and apply, but don't commit, fixes
cmake --build build --target format

# Run clang-format over the entire codebase and return an error if changes are still needed
cmake --build build --target check-format   

# "One-shot" style fixes: Runs `format` followed by `tidy` followed by `check-tidy`
cmake --build build --target style

Does this PR introduce breaking changes? What changes might users need to make to their code?

Introduces no breaking changes.

Does this PR change default behavior?

Changes no default behavior.

@nathanwbrei
Copy link
Copy Markdown
Contributor Author

Curiously, the CMake-ized clang-tidy emits a bunch of warnings on the unmodified codebase, even though it is using the provided config file. A lot of the warnings are from ACTS headers, but there's 22 files in the EICrecon source tree that got reasonable-looking fixes. So I'm curious what CI is doing differently.

@nathanwbrei nathanwbrei requested a review from veprbl February 20, 2026 03:36
@wdconinc
Copy link
Copy Markdown
Contributor

So I'm curious what CI is doing differently.

Maybe you already figured this out, but CI only runs on changed files. People don't want to get issues flagged in files they didn't change. Over time, since we introduce this with clang-16 or so, this means that some newer clang-20 checks are not satisfied by code that hasn't been touched recently.

A lot of the warnings are from ACTS headers

In CI we suppress issues in headers that are not under our version control.

@@ -0,0 +1,64 @@
find_program(RUN_CLANG_TIDY run-clang-tidy-19)
Copy link
Copy Markdown
Contributor

@wdconinc wdconinc Feb 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eic-shell has clang-20 so this will fail.

Comment thread CMakeLists.txt
# Find<Modules>.cmake

# Expose clang-tidy and clang-format as (phony) CMake targets
include(cmake/CodeStyleTargets.cmake)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy integration in CMake is already (bare bones) provided at

# Also use clang-tidy integration in CMake
. It might make sense to align this new entry point with that approach (which uses built-in CMake support for tools like this, not git ls-files).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants