-
-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Add parallel execution support for clang-tidy #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
d01022f
6b15bef
0252a86
93848ed
ae625e6
fc4f61f
432f369
2c05970
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,48 @@ | ||
| from concurrent.futures import ThreadPoolExecutor | ||
| import subprocess | ||
| import sys | ||
| from argparse import ArgumentParser | ||
| from argparse import ArgumentParser, ArgumentTypeError | ||
| from pathlib import Path | ||
| from typing import Optional, Tuple | ||
| from typing import List, Optional, Tuple | ||
|
|
||
| from cpp_linter_hooks.util import resolve_install, DEFAULT_CLANG_TIDY_VERSION | ||
|
|
||
| COMPILE_DB_SEARCH_DIRS = ["build", "out", "cmake-build-debug", "_build"] | ||
| SOURCE_FILE_SUFFIXES = { | ||
| ".c", | ||
| ".cc", | ||
| ".cp", | ||
| ".cpp", | ||
| ".cxx", | ||
| ".c++", | ||
| ".cu", | ||
| ".cuh", | ||
| ".h", | ||
| ".hh", | ||
| ".hpp", | ||
| ".hxx", | ||
| ".h++", | ||
| ".ipp", | ||
| ".inl", | ||
| ".ixx", | ||
| ".tpp", | ||
| ".txx", | ||
| } | ||
|
|
||
|
|
||
| def _positive_int(value: str) -> int: | ||
| jobs = int(value) | ||
| if jobs < 1: | ||
| raise ArgumentTypeError("--jobs must be greater than 0") | ||
| return jobs | ||
|
|
||
| parser = ArgumentParser() | ||
| parser.add_argument("--version", default=DEFAULT_CLANG_TIDY_VERSION) | ||
| parser.add_argument("--compile-commands", default=None, dest="compile_commands") | ||
| parser.add_argument( | ||
| "--no-compile-commands", action="store_true", dest="no_compile_commands" | ||
| ) | ||
| parser.add_argument("-j", "--jobs", type=_positive_int, default=1) | ||
| parser.add_argument("-v", "--verbose", action="store_true") | ||
|
|
||
|
|
||
|
|
@@ -74,6 +103,38 @@ def _exec_clang_tidy(command) -> Tuple[int, str]: | |
| return 1, str(e) | ||
|
|
||
|
|
||
| def _looks_like_source_file(path: str) -> bool: | ||
| return any(suffix.lower() in SOURCE_FILE_SUFFIXES for suffix in Path(path).suffixes) | ||
|
shenxianpeng marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
||
| def _split_source_files(args: List[str]) -> Tuple[List[str], List[str]]: | ||
| split_idx = len(args) | ||
| source_files: List[str] = [] | ||
| for idx in range(len(args) - 1, -1, -1): | ||
| if not _looks_like_source_file(args[idx]): | ||
| break | ||
| source_files.append(args[idx]) | ||
| split_idx = idx | ||
| return args[:split_idx], list(reversed(source_files)) | ||
|
|
||
|
|
||
| def _combine_outputs(results: List[Tuple[int, str]]) -> str: | ||
| return "\n".join(output.rstrip("\n") for _, output in results if output) | ||
|
|
||
|
|
||
| def _exec_parallel_clang_tidy( | ||
| command_prefix: List[str], source_files: List[str], jobs: int | ||
| ) -> Tuple[int, str]: | ||
| def run_file(source_file: str) -> Tuple[int, str]: | ||
| return _exec_clang_tidy(command_prefix + [source_file]) | ||
|
|
||
| with ThreadPoolExecutor(max_workers=min(jobs, len(source_files))) as executor: | ||
| results = list(executor.map(run_file, source_files)) | ||
|
|
||
| retval = 1 if any(retval != 0 for retval, _ in results) else 0 | ||
| return retval, _combine_outputs(results) | ||
|
|
||
|
|
||
| def run_clang_tidy(args=None) -> Tuple[int, str]: | ||
| hook_args, other_args = parser.parse_known_args(args) | ||
| if hook_args.version: | ||
|
|
@@ -90,6 +151,12 @@ def run_clang_tidy(args=None) -> Tuple[int, str]: | |
| ) | ||
| other_args = ["-p", compile_db_path] + other_args | ||
|
|
||
| clang_tidy_args, source_files = _split_source_files(other_args) | ||
| if hook_args.jobs > 1 and len(source_files) > 1: | ||
|
shenxianpeng marked this conversation as resolved.
Outdated
|
||
| return _exec_parallel_clang_tidy( | ||
| ["clang-tidy"] + clang_tidy_args, source_files, hook_args.jobs | ||
| ) | ||
|
Comment on lines
+157
to
+168
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 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:
Treat the This branch only falls back for 🛡️ 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 Replace with (RUF005) 🤖 Prompt for AI Agents |
||
|
|
||
| return _exec_clang_tidy(["clang-tidy"] + other_args) | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import pytest | ||
| import subprocess | ||
| import time | ||
| from pathlib import Path | ||
| from unittest.mock import patch, MagicMock | ||
|
|
||
|
|
@@ -227,3 +228,71 @@ def test_no_verbose_no_extra_stderr(tmp_path, monkeypatch, capsys): | |
| ): | ||
| run_clang_tidy(["dummy.cpp"]) | ||
| assert capsys.readouterr().err == "" | ||
|
|
||
|
|
||
| def test_jobs_one_keeps_single_invocation(): | ||
| 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=1", "-p", "./build", "a.cpp", "b.cpp"]) | ||
|
|
||
| mock_exec.assert_called_once_with( | ||
| ["clang-tidy", "-p", "./build", "a.cpp", "b.cpp"] | ||
| ) | ||
|
|
||
|
|
||
| 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" | ||
|
|
||
|
Comment on lines
+245
to
+252
|
||
| with ( | ||
| patch("cpp_linter_hooks.clang_tidy._exec_clang_tidy", side_effect=fake_exec), | ||
| patch("cpp_linter_hooks.clang_tidy.resolve_install"), | ||
| ): | ||
| ret, output = run_clang_tidy( | ||
| [ | ||
| "--jobs=4", | ||
| "-p", | ||
| "./build", | ||
| "--export-fixes", | ||
| "fixes.yaml", | ||
| "a.cpp", | ||
| "b.cpp", | ||
| ] | ||
| ) | ||
|
|
||
| assert ret == 1 | ||
| assert output == "a.cpp output\nb.cpp output" | ||
|
|
||
|
|
||
| def test_jobs_parallelizes_only_trailing_source_files(): | ||
| 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=2", | ||
| "-p", | ||
| "./build", | ||
| "--export-fixes", | ||
| "fixes.yaml", | ||
| "a.cpp", | ||
| "b.hpp", | ||
| ] | ||
| ) | ||
|
|
||
| commands = {tuple(call.args[0]) for call in mock_exec.call_args_list} | ||
| assert commands == { | ||
| ("clang-tidy", "-p", "./build", "--export-fixes", "fixes.yaml", "a.cpp"), | ||
| ("clang-tidy", "-p", "./build", "--export-fixes", "fixes.yaml", "b.hpp"), | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.