feat: add support for compilation databases in clang-tidy#201
feat: add support for compilation databases in clang-tidy#201shenxianpeng merged 5 commits intomainfrom
Conversation
github-actions
bot
added
the
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
+ Coverage 95.72% 96.59% +0.87%
==========================================
Files 4 4
Lines 117 147 +30
==========================================
+ Hits 112 142 +30
Misses 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 31 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdded clang-tidy compilation database support with auto-detection of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_clang_tidy.py (1)
29-35:⚠️ Potential issue | 🟡 MinorTest does not use
tmp_pathfixture correctly.The test writes to
testing/main.cinstead of copying the file totmp_path. This violates the coding guideline and modifies repository files during tests.🛠️ Proposed fix
-def test_run_clang_tidy_valid(args, expected_retval): - # copy test file to tmp_path to prevent modifying repo data - test_file = Path("testing/main.c") - test_file.write_bytes(Path("testing/main.c").read_bytes()) +def test_run_clang_tidy_valid(args, expected_retval, tmp_path): + # copy test file to tmp_path to prevent modifying repo data + test_file = tmp_path / "main.c" + test_file.write_bytes(Path("testing/main.c").read_bytes()) ret, output = run_clang_tidy(args + [str(test_file)])As per coding guidelines: "Use
tmp_pathfixture in tests to avoid modifying repository files".🤖 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 29 - 35, The test test_run_clang_tidy_valid modifies the repo by writing to testing/main.c instead of using the tmp_path fixture; fix it by copying the source file into tmp_path and pointing test_file at that copy before calling run_clang_tidy (use tmp_path / "main.c" and copy bytes from Path("testing/main.c") into it), then call ret, output = run_clang_tidy(args + [str(test_file)]) and assert as before; update any variable names (test_file) accordingly so the test no longer touches repository files.
🧹 Nitpick comments (4)
tests/test_clang_tidy.py (2)
64-68: Remove unused_patch()helper function.This helper is defined but never called—tests inline the patching directly. Remove to avoid dead code.
🤖 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 64 - 68, Remove the unused helper function _patch(): delete the entire def _patch() block (including its return tuple of patch(...) calls) from tests/test_clang_tidy.py since it's never invoked; ensure no remaining references to _patch() exist and run tests to confirm nothing else relies on it.
61-61: Shared mutableMagicMockmay cause test interference.
_MOCK_RUNis a module-level mutable object. If any test mutates attributes on it, subsequent tests could be affected. Consider creating a fresh mock per test or usingspecto prevent attribute mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_clang_tidy.py` at line 61, _MOCK_RUN is defined as a shared module-level MagicMock which can be mutated by tests and cause cross-test interference; replace it with a factory or fixture that returns a fresh MagicMock(returncode=0, stdout="", stderr="") for each test (or create the mock with spec/autospec if you need to prevent setting arbitrary attributes). Update tests that currently reference _MOCK_RUN to call the factory/fixture (or to use the per-test mock) and ensure any patching uses the new per-test instance so no state is shared across tests.cpp_linter_hooks/clang_tidy.py (2)
55-55: Use list unpacking syntax.Per Ruff RUF005, prefer unpacking over concatenation for readability.
- other_args = ["-p", compile_db_path] + other_args + other_args = ["-p", compile_db_path, *other_args]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp_linter_hooks/clang_tidy.py` at line 55, Replace the list concatenation that prepends the compile DB args to other_args by constructing a new list that starts with the "-p" marker and compile_db_path and then unpacks the existing other_args into it (i.e., use Python list unpacking with * to include the current other_args). Update the assignment to other_args so it uses this unpacking approach instead of using the + operator; refer to the other_args variable and compile_db_path in clang_tidy.py when making the change.
25-56: Refactor to reduce cognitive complexity.SonarCloud flags this function at 23 complexity vs. 15 allowed. Consider extracting the compile-commands resolution logic into a helper function.
♻️ Suggested refactor
+def _resolve_compile_db(hook_args, other_args) -> Tuple[Optional[str], Optional[Tuple[int, str]]]: + """Resolve compile database path. Returns (path, error) where error is a return tuple if invalid.""" + has_p = any(a == "-p" or a.startswith("-p=") for a in other_args) + + if hook_args.no_compile_commands: + return None, None + + if hook_args.compile_commands: + if has_p: + print("Warning: --compile-commands ignored; -p already in args", file=sys.stderr) + return None, None + p = Path(hook_args.compile_commands) + if not p.is_dir() or not (p / "compile_commands.json").exists(): + return None, (1, f"--compile-commands: no compile_commands.json in '{hook_args.compile_commands}'") + return hook_args.compile_commands, None + + if not has_p: + return _find_compile_commands(), None + return None, None + + def run_clang_tidy(args=None) -> Tuple[int, str]: hook_args, other_args = parser.parse_known_args(args) if hook_args.version: resolve_install("clang-tidy", hook_args.version) - # Covers both "-p ./build" (two tokens) and "-p=./build" (one token) - has_p = any(a == "-p" or a.startswith("-p=") for a in other_args) - - compile_db_path = None - if not hook_args.no_compile_commands: - if hook_args.compile_commands: - if has_p: - print( - "Warning: --compile-commands ignored; -p already in args", - file=sys.stderr, - ) - else: - p = Path(hook_args.compile_commands) - if not p.is_dir() or not (p / "compile_commands.json").exists(): - return 1, ( - f"--compile-commands: no compile_commands.json" - f" in '{hook_args.compile_commands}'" - ) - compile_db_path = hook_args.compile_commands - elif not has_p: - compile_db_path = _find_compile_commands() + compile_db_path, error = _resolve_compile_db(hook_args, other_args) + if error: + return error if compile_db_path: if hook_args.verbose: print(f"Using compile_commands.json from: {compile_db_path}", file=sys.stderr) - other_args = ["-p", compile_db_path] + other_args + other_args = ["-p", compile_db_path, *other_args]🤖 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 25 - 56, The run_clang_tidy function has high cognitive complexity due to inline compile-commands resolution; extract that logic into a new helper function (e.g., resolve_compile_db_path(hook_args, other_args)) that returns (compile_db_path, warning_message_or_none, error_tuple_or_none). Move all checks around hook_args.no_compile_commands, hook_args.compile_commands, has_p detection, validation of compile_commands dir and _find_compile_commands call into that helper, and have run_clang_tidy call it to receive compile_db_path and handle printing warnings/errors and prepending ["-p", compile_db_path] to other_args; keep references to parser, hook_args, other_args, _find_compile_commands and preserve current stderr prints and return behavior.
🤖 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 1-5: CI failed because files (including
cpp_linter_hooks/clang_tidy.py) were reformatted by ruff-format; run the ruff
formatter locally across the repo (e.g., format the changed files or entire
project), stage the resulting changes, and commit them so the formatted version
of clang_tidy.py and any other modified files are included in the PR.
---
Outside diff comments:
In `@tests/test_clang_tidy.py`:
- Around line 29-35: The test test_run_clang_tidy_valid modifies the repo by
writing to testing/main.c instead of using the tmp_path fixture; fix it by
copying the source file into tmp_path and pointing test_file at that copy before
calling run_clang_tidy (use tmp_path / "main.c" and copy bytes from
Path("testing/main.c") into it), then call ret, output = run_clang_tidy(args +
[str(test_file)]) and assert as before; update any variable names (test_file)
accordingly so the test no longer touches repository files.
---
Nitpick comments:
In `@cpp_linter_hooks/clang_tidy.py`:
- Line 55: Replace the list concatenation that prepends the compile DB args to
other_args by constructing a new list that starts with the "-p" marker and
compile_db_path and then unpacks the existing other_args into it (i.e., use
Python list unpacking with * to include the current other_args). Update the
assignment to other_args so it uses this unpacking approach instead of using the
+ operator; refer to the other_args variable and compile_db_path in
clang_tidy.py when making the change.
- Around line 25-56: The run_clang_tidy function has high cognitive complexity
due to inline compile-commands resolution; extract that logic into a new helper
function (e.g., resolve_compile_db_path(hook_args, other_args)) that returns
(compile_db_path, warning_message_or_none, error_tuple_or_none). Move all checks
around hook_args.no_compile_commands, hook_args.compile_commands, has_p
detection, validation of compile_commands dir and _find_compile_commands call
into that helper, and have run_clang_tidy call it to receive compile_db_path and
handle printing warnings/errors and prepending ["-p", compile_db_path] to
other_args; keep references to parser, hook_args, other_args,
_find_compile_commands and preserve current stderr prints and return behavior.
In `@tests/test_clang_tidy.py`:
- Around line 64-68: Remove the unused helper function _patch(): delete the
entire def _patch() block (including its return tuple of patch(...) calls) from
tests/test_clang_tidy.py since it's never invoked; ensure no remaining
references to _patch() exist and run tests to confirm nothing else relies on it.
- Line 61: _MOCK_RUN is defined as a shared module-level MagicMock which can be
mutated by tests and cause cross-test interference; replace it with a factory or
fixture that returns a fresh MagicMock(returncode=0, stdout="", stderr="") for
each test (or create the mock with spec/autospec if you need to prevent setting
arbitrary attributes). Update tests that currently reference _MOCK_RUN to call
the factory/fixture (or to use the per-test mock) and ensure any patching uses
the new per-test instance so no state is shared across tests.
🪄 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: bd04d263-46bc-4e95-ab09-17843b968303
📒 Files selected for processing (3)
README.mdcpp_linter_hooks/clang_tidy.pytests/test_clang_tidy.py
Merging this PR will degrade performance by 16.55%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | test_run_clang_tidy_valid[args2-1] |
1.1 ms | 1.2 ms | -13.48% |
| ❌ | test_run_clang_tidy_valid[args5-1] |
890.7 µs | 1,052.1 µs | -15.34% |
| ❌ | test_run_clang_tidy_valid[args6-1] |
892.6 µs | 1,060.5 µs | -15.84% |
| ❌ | test_run_clang_tidy_valid[args4-1] |
1.1 ms | 1.2 ms | -13.29% |
| ❌ | test_run_clang_tidy_invalid[args5-1] |
837.8 µs | 1,002.9 µs | -16.47% |
| ❌ | test_run_clang_tidy_invalid[args0-1] |
876.7 µs | 1,031.1 µs | -14.98% |
| ❌ | test_run_clang_tidy_invalid[args6-1] |
847.9 µs | 1,012.4 µs | -16.24% |
| ❌ | test_run_clang_tidy_valid[args0-1] |
872.7 µs | 1,042.6 µs | -16.3% |
| ❌ | test_run_clang_tidy_invalid[args1-1] |
998.6 µs | 1,167.2 µs | -14.44% |
| ❌ | test_run_clang_tidy_invalid[args4-1] |
1 ms | 1.2 ms | -14.18% |
| ❌ | test_run_clang_tidy_invalid[args2-1] |
995.6 µs | 1,163.6 µs | -14.44% |
| ❌ | test_run_clang_tidy_valid[args1-1] |
1.1 ms | 1.2 ms | -13.74% |
| ❌ | test_run_clang_tidy_valid[args3-1] |
885.1 µs | 1,053.1 µs | -15.95% |
| ❌ | test_run_clang_tidy_invalid[args3-1] |
842.7 µs | 1,009.8 µs | -16.55% |
Comparing fix-197 (8a0b41a) with main (451e2e0)2
Footnotes
-
13 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(b29757a) during the generation of this report, so 451e2e0 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
|
shenxianpeng
added
minor
1 participant
closes #197
Summary by CodeRabbit
Release Notes
New Features
compile_commands.jsonin common build directories--compile-commandsCLI option to explicitly specify the database directory--no-compile-commandsflag to disable automatic detection-v/--verboseoption for detailed logging outputDocumentation