Repository navigation
fix: shift changed lines past clang-tidy fixes above them - #491
Merged
Merged
Conversation
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 57 minutes. View limit details
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 |
When a clang-tidy fix lands entirely above a changed line range, `three_way_diff()` added the fix's line delta to `line_shift` but kept the range's start and end computed before the loop. The range passed to clang-format then pointed at the old line numbers, so format advice was filtered against the wrong lines in `lines-changed-only` mode. Add a test for that case, which also covers the last uncovered lines in clang_format.rs.
shenxianpeng
force-pushed
the
claude/rs-test-coverage-7cgkd7
branch
from
October 15, 2026 19:31
f1ebcf0 to
af4ab25
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #491 +/- ##
==========================================
+ Coverage 99.18% 99.25% +0.07%
==========================================
Files 26 26
Lines 5653 5664 +11
==========================================
+ Hits 5607 5622 +15
+ Misses 46 42 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2bndy5
approved these changes
Oct 15, 2026
2bndy5
left a comment
2bndy5
left a comment
Collaborator
There was a problem hiding this comment.
Good catch! I'll pay closer attention to indirect coverage changes from now on.
BTW, I'm glad to get a second pair of eyes on that 3-way-diff function. My dreams turned into chains of boolean decisions when I was writing, reviewing, rewriting, and re-reviewing that patch.
2bndy5
pushed a commit
that referenced
this pull request
Oct 15, 2026
<!-- claude-projects-attribution: {"github_login":"shenxianpeng"} -->
Before: compared with the Python cpp-linter (v1), v2 shows clang-tidy
annotations on the whole file instead of the line, posts an approving PR
review even with `--no-lgtm` (the default), posts a new review when
every suggestion was already posted, lists files in a random order, lets
one failed check pass under `PRE_COMMIT=1`, and rejects or mishandles a
few inputs v1 accepts (`-e ".cpp, .h"`, `--extra-arg` with double
spaces, `-v 10`, `CPP_LINTER_PR_REVIEW_SUMMARY_ONLY=True`, meson-style
relative paths in clang-tidy output).
After: v2 behaves like v1 in those places. Intentional v2 changes
(`--pr-review`, parallel by default, SARIF, the fix patch, #488's header
fix) are untouched.
I compared both CLIs, defaults, env vars, file filtering and every
feedback path (annotations, step summary, thread comment, PR review,
outputs, exit code). This PR only fixes the clear gaps. Differences that
look deliberate or need a maintainer's call (for example
`lines-changed-only` defaulting to `true`, the swapped `-t`/`-g` short
flags, no `--jobs`/`--fix`) are left out and listed in a separate report
for @2bndy5 to weigh in on.
## Changes
- **Annotations**: clang-tidy annotations set
`start_line`/`start_column`. Before, only `end_line`/`end_column` were
set, and git-bot-feedback only writes `line=` from `start_line`, so the
annotation had no line. The title also names the check, as in v1.
- **`--no-lgtm` for PR reviews**: restored the check lost in the
git-bot-feedback migration (d1e2136). Outdated reviews are still culled;
only the approving review is skipped.
- **Reused review comments**: when every review comment was already
posted in a previous review, no new review is posted (v1.15
`rest_api.py` L468-473).
- **Stable file order**: the file lists from git-bot-feedback
(`HashMap`/`HashSet`) are sorted, as v1 does.
- **`PRE_COMMIT=1`**: `checks_failed > 1` → `> 0`.
- **`--extensions`**: trims whitespace and a leading `.`; empty values
are still an error.
- **`--extra-arg`**: splits on any whitespace and drops empty values, so
clang-tidy never gets `--extra-arg=`.
- **`--verbosity`**: accepts `10` (debug) and `20` (info), like v1.
- **`CPP_LINTER_PR_REVIEW_SUMMARY_ONLY`**: compared case-insensitively.
- **Compilation database paths**: a clang-tidy path that equals a unit's
`file` (relative to its `directory`, as meson writes it) now resolves
through that unit, like v1.
- **Review diagnostics**: unfixed clang-tidy diagnostics about another
file (eg. a header) are no longer posted on the analyzed file's lines
(v1 has this bug too).
- **Local runs**: warn when `--pr-review` is set outside CI, like v1.
- **Thread comment**: `clang-tidy (vX) reports: <strong>N
concern(s)</strong>`, matching the clang-format line.
- **`--style` help**: no longer says the style is passed to clang-tidy
(it isn't since #488).
Nothing here touches `clang_format.rs` (#491) or the fix-application
code planned for #489/#490.
## How
Each fix has a test. The `no_lgtm` review test now asserts no review is
POSTed, and it and `summary_only` (now using `True`) fail on `main` and
pass here. The new `all_suggestions_reused` test runs a review once,
feeds its comments back as existing review threads, and asserts the
second run posts nothing and keeps the old review; it fails without the
fix. `cargo clippy --all-features --all-targets` is clean and `cargo
nextest run --all-features -p cpp-linter` passes locally (124 tests,
clang 18).
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved clang-tidy diagnostic matching for compilation database paths
and excluded diagnostics for other files from suggestions.
* Tidy review annotations now point to the diagnostic’s location and
include clearer titles and summaries.
* Pre-commit runs now fail when any check fails, and file feedback
appears in a consistent order.
* **Review Improvements**
* Avoided posting duplicate review comments and LGTM reviews when LGTM
is disabled.
* Added a warning when pull-request reviews are requested outside a
supported CI environment.
* **CLI Improvements**
* Normalized extension and extra-argument input, and accepted additional
verbosity and mixed-case summary-only values.
* Clarified that the clang-format style also applies to clang-tidy
suggestions.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before: when clang-tidy fixes and clang-format both run, and a clang-tidy fix adds or removes lines entirely above a changed line range, the range given to clang-format keeps its old line numbers. With
lines-changed-only, format advice is then filtered against the wrong lines.After: the changed range is shifted by the lines that fix added or removed, so clang-format checks the lines that actually changed.
This came out of a coverage pass on main after #488. The "tidy hunk is before the og range" branch of
three_way_diff()was the only uncovered code inclang_format.rs, and its first test failed: for an original range5..=6with a fix that turns line 2 into two lines, it returned[2..=3, 5..=6]instead of[2..=3, 6..=7].How: that branch updated
line_shiftbut never recomputedmerged_start/merged_end, which are set once before the loop. It now recomputes them after shifting, the same way the "contained within" branch recomputesmerged_end. The new testthree_way_diff_tidy_hunk_before_rangecovers it.Coverage notes, for context (local Linux run, nothing else changed here):
clang_format.rsnow has no uncovered lines. The other uncovered lines on main are platform-only branches (Windows/macOS/arm64), error arms that can't fail (writing to aString), and the other-file branch inclang_tidy/mod.rsthat #489/#490 will rewrite, so it is left alone.