feat: Add parallel execution support for clang-tidy#203
Conversation
github-actions
bot
added
the
documentation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #203 +/- ##
==========================================
+ Coverage 96.59% 96.64% +0.04%
==========================================
Files 4 4
Lines 147 179 +32
==========================================
+ Hits 142 173 +31
- Misses 5 6 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shenxianpeng
added
enhancement
shenxianpeng
changed the title
There was a problem hiding this comment.
Pull request overview
Adds parallel execution support to the clang-tidy pre-commit hook by introducing a --jobs/-j option, aiming to speed up runs across multiple source files while keeping serial behavior for single-file usage or --jobs=1.
Changes:
- Add
--jobs/-jparsing and parallel dispatch incpp_linter_hooks/clang_tidy.py. - Extend
tests/test_clang_tidy.pywith cases for serial fallback, parallel dispatch, and trailing source-file splitting. - Document
--jobsusage forclang-tidyin the README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cpp_linter_hooks/clang_tidy.py |
Introduces --jobs and implements per-file parallel clang-tidy execution with ordered output aggregation. |
tests/test_clang_tidy.py |
Adds tests validating --jobs=1 behavior, parallel output ordering, and trailing-file detection. |
README.md |
Documents how to enable parallel processing via --jobs/-j in pre-commit config. |
| def test_jobs_parallelizes_source_files_and_preserves_output_order(): | ||
| def fake_exec(command): | ||
| source_file = command[-1] | ||
| if source_file == "a.cpp": | ||
| time.sleep(0.05) | ||
| return 0, "a.cpp output" | ||
| return 1, "b.cpp output" | ||
|
|
There was a problem hiding this comment.
This test relies on time.sleep() to try to force out-of-order completion, but thread scheduling can still allow a.cpp to complete before b.cpp, making the ordering assertion non-deterministic (and potentially missing regressions if output were combined by completion order). Using synchronization (e.g., threading.Event/Barrier) to guarantee b.cpp finishes first would make the test reliable.
Merging this PR will not alter performance
Comparing 
Footnotes
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
github-actions
bot
added
the
documentation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
WalkthroughThis pull request adds parallel execution support to clang-tidy via a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_clang_tidy.py (1)
297-326: Parametrize this fallback test for both--export-fixessyntaxes.
unsafe_parallelchecks botharg == "--export-fixes"andarg.startswith("--export-fixes="), but this case only exercises the two-token form.🧪 Suggested parametrization
+@pytest.mark.parametrize( + ("export_fixes_args", "expected_export_fixes_args"), + ( + (("--export-fixes", "fixes.yaml"), ("--export-fixes", "fixes.yaml")), + (("--export-fixes=fixes.yaml",), ("--export-fixes=fixes.yaml",)), + ), +) -def test_jobs_with_export_fixes_forces_serial_execution(): +def test_jobs_with_export_fixes_forces_serial_execution( + export_fixes_args, expected_export_fixes_args +): with ( patch( "cpp_linter_hooks.clang_tidy._exec_clang_tidy", return_value=(0, "") ) as mock_exec, patch("cpp_linter_hooks.clang_tidy.resolve_install"), ): run_clang_tidy( [ "--jobs=4", "-p", "./build", - "--export-fixes", - "fixes.yaml", + *export_fixes_args, "a.cpp", "b.cpp", ] ) mock_exec.assert_called_once_with( [ "clang-tidy", "-p", "./build", - "--export-fixes", - "fixes.yaml", + *expected_export_fixes_args, "a.cpp", "b.cpp", ] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_clang_tidy.py` around lines 297 - 326, Update the test_jobs_with_export_fixes_forces_serial_execution to run for both forms of the export-fixes flag (the two-token form ["--export-fixes", "fixes.yaml"] and the single-token form ["--export-fixes=fixes.yaml"]) because unsafe_parallel checks both arg == "--export-fixes" and arg.startswith("--export-fixes="). Change the test to parametrize (or loop) over the two input arg variants passed to run_clang_tidy and assert that mock_exec.assert_called_once_with is invoked with the expected flattened argv (i.e., the clang-tidy call contains "--export-fixes", "fixes.yaml" in the two-token case and "--export-fixes=fixes.yaml" in the single-token case), keeping the same mocks for _exec_clang_tidy and resolve_install and retaining the test name test_jobs_with_export_fixes_forces_serial_execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp_linter_hooks/clang_tidy.py`:
- Around line 157-168: The parallel-safety check only blocks "--export-fixes"
but must also treat clang-tidy's fixing modes as unsafe; update the
unsafe_parallel predicate (which examines clang_tidy_args) to also return true
when any arg enables fixes (e.g., arg == "--fix" or arg.startswith("--fix") /
arg.startswith("--fix-") or arg.startswith("--fix=") and similarly cover
"--fix-errors" and "--fix-notes"), so that when hook_args.jobs > 1 and multiple
source_files the code will fall back to the serial path via
_exec_parallel_clang_tidy instead of running concurrently when fix flags are
present.
---
Nitpick comments:
In `@tests/test_clang_tidy.py`:
- Around line 297-326: Update the
test_jobs_with_export_fixes_forces_serial_execution to run for both forms of the
export-fixes flag (the two-token form ["--export-fixes", "fixes.yaml"] and the
single-token form ["--export-fixes=fixes.yaml"]) because unsafe_parallel checks
both arg == "--export-fixes" and arg.startswith("--export-fixes="). Change the
test to parametrize (or loop) over the two input arg variants passed to
run_clang_tidy and assert that mock_exec.assert_called_once_with is invoked with
the expected flattened argv (i.e., the clang-tidy call contains
"--export-fixes", "fixes.yaml" in the two-token case and
"--export-fixes=fixes.yaml" in the single-token case), keeping the same mocks
for _exec_clang_tidy and resolve_install and retaining the test name
test_jobs_with_export_fixes_forces_serial_execution.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07e797fb-a3eb-4cde-9c4d-3a71ed6df506
📒 Files selected for processing (3)
README.mdcpp_linter_hooks/clang_tidy.pytests/test_clang_tidy.py
| # Parallel execution is unsafe when arguments include flags that write to a | ||
| # shared output path (e.g., --export-fixes fixes.yaml). In that case, force | ||
| # serial execution to avoid concurrent writes/overwrites. | ||
| unsafe_parallel = any( | ||
| arg == "--export-fixes" or arg.startswith("--export-fixes=") | ||
| for arg in clang_tidy_args | ||
| ) | ||
|
|
||
| if hook_args.jobs > 1 and len(source_files) > 1 and not unsafe_parallel: | ||
| return _exec_parallel_clang_tidy( | ||
| ["clang-tidy"] + clang_tidy_args, source_files, hook_args.jobs | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
clang-tidy command line reference: do --fix, --fix-errors, and --fix-notes modify source/header files, and are concurrent invocations safe when multiple translation units include the same header?
💡 Result:
Yes, --fix, --fix-errors, and --fix-notes modify the source and header files in place. No, concurrent invocations are not safe when multiple translation units include the same header, as parallel processes can generate conflicting fixes for the shared header, leading to race conditions during file writes.
Citations:
- 1: https://clang.llvm.org/extra/clang-tidy/
- 2: https://clang.llvm.org/extra/doxygen/ClangTidyMain_8cpp_source.html
- 3: https://clang.llvm.org/extra/doxygen/run-clang-tidy_8py_source.html
- 4: https://stackoverflow.com/questions/35261868/getting-clang-tidy-to-fix-header-files
- 5: https://stackoverflow.com/questions/79078050/applying-clang-tidys-readability-identifier-naming-fix-corrects-only-the-header
- 6: https://raw.githubusercontent.com/llvm-mirror/clang-tools-extra/master/clang-tidy/tool/run-clang-tidy.py
- 7: Interest in updated run-clang-tidy, with header error deduplication? llvm/llvm-project#91641
- 8: Should clang-tidy support multiple threads? llvm/llvm-project#58371
Treat the --fix* modes as unsafe here too.
This branch only falls back for --export-fixes. At minimum, --fix, --fix-errors, and --fix-notes also need the same serial fallback because they can rewrite files shared across translation units and make the hook nondeterministic.
🛡️ Suggested guard expansion
- unsafe_parallel = any(
- arg == "--export-fixes" or arg.startswith("--export-fixes=")
- for arg in clang_tidy_args
- )
+ unsafe_parallel_flags = {
+ "--export-fixes",
+ "--fix",
+ "--fix-errors",
+ "--fix-notes",
+ }
+ unsafe_parallel = any(
+ arg in unsafe_parallel_flags or arg.startswith("--export-fixes=")
+ for arg in clang_tidy_args
+ )🧰 Tools
🪛 Ruff (0.15.7)
[warning] 167-167: Consider ["clang-tidy", *clang_tidy_args] instead of concatenation
Replace with ["clang-tidy", *clang_tidy_args]
(RUF005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp_linter_hooks/clang_tidy.py` around lines 157 - 168, The parallel-safety
check only blocks "--export-fixes" but must also treat clang-tidy's fixing modes
as unsafe; update the unsafe_parallel predicate (which examines clang_tidy_args)
to also return true when any arg enables fixes (e.g., arg == "--fix" or
arg.startswith("--fix") / arg.startswith("--fix-") or arg.startswith("--fix=")
and similarly cover "--fix-errors" and "--fix-notes"), so that when
hook_args.jobs > 1 and multiple source_files the code will fall back to the
serial path via _exec_parallel_clang_tidy instead of running concurrently when
fix flags are present.
|
@copilot add e2e test to testing folder. for example add more |
Agent-Logs-Url: https://github.com/cpp-linter/cpp-linter-hooks/sessions/d3b34e23-651a-4719-8b0f-3a2b57ba280e Co-authored-by: shenxianpeng <3353385+shenxianpeng@users.noreply.github.com>
Agent-Logs-Url: https://github.com/cpp-linter/cpp-linter-hooks/sessions/d3b34e23-651a-4719-8b0f-3a2b57ba280e Co-authored-by: shenxianpeng <3353385+shenxianpeng@users.noreply.github.com>
Added in commit 2c05970:
|
|
closes #198
Summary
-j/--jobsto theclang-tidyhook to run multiple source files in parallel--jobs=1--export-fixesis present to prevent concurrent file writes--export-fixesserial fallbacktesting/pre-commit-config-parallel.yamland a parallel test section intesting/run.shthat exercises--jobs=2against multiple source filesTesting
uv run pytest tests/test_clang_tidy.pyuv run pytesttesting/run.sh(includes a parallel e2e test againsttesting/main.candtesting/good.c)Summary by CodeRabbit
New Features
-j/--jobscommand-line option to enable parallel execution of clang-tidy across multiple source files, improving performance on multi-core systems.Documentation