feat: merge review suggestions from both clang tools#358
Open
2bndy5 wants to merge 1 commit into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
==========================================
- Coverage 92.81% 92.43% -0.38%
==========================================
Files 23 23
Lines 3450 3528 +78
==========================================
+ Hits 3202 3261 +59
- Misses 248 267 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2bndy5
force-pushed
the
join-tools-reviews
branch
3 times, most recently
from
b5e7407 to
e48fbfc
Compare
This is a culmination of - #354 - #347 It it the last step needed before providing a patch that consumers can push to auto-fix lints. # Intended flow of data 1. run clang-tidy before running clang-format 2. after running clang-tidy save the patched file to cache 3. after running clang-format: - check for clang-tidy patch in cache. - if cached patch is present, run clang-format on the clang-tidy patch. This step includes formatting lines that clang-tidy changed and lines that clang-format changed. - if cached patch is not present (clang-tidy was not run on the file), then cache the clang-format fixes instead. - `--lines-changed-only` is respected, but this may need tweaking in the future. 4. when creating a PR review, only add hunks that are not present in the review comments. I added a parameter to also calculate (and display) how many review comments were reused in previous PR reviews. If PR review is a summary only, then the patch will include all fixes that would have been posted in the diff. Adjusted tests accordingly.
2bndy5
force-pushed
the
join-tools-reviews
branch
from
dd4c541 to
9254f94
Compare
Contributor
WalkthroughThe PR refactors the clang tool pipeline to consolidate cached patched-file ownership into a new ChangesUnified patched_path & ReviewComments refactor
Docs dependency bump
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp-linter/src/clang_tools/clang_format.rs`:
- Around line 154-185: The joint_ranges are built using hunk.before which
contains original-file line coordinates, but the code is formatting the cached
tidy-patched file where line numbers have shifted due to tidy
insertions/deletions. Replace hunk.before with hunk.after in the map function
that builds joint_ranges to use the correct patched-file coordinates.
Additionally, translate any non-overlapping original ranges from the ranges
parameter before adding them to joint_ranges, since they also refer to
original-file coordinates and must be converted to patched-file coordinates
before being passed as --lines arguments to clang-format.
In `@cpp-linter/src/common_fs.rs`:
- Around line 261-263: The UnifiedDiffPrinter's display_hunk() method requires
that display_header() be called first to emit the hunk header line (the @@ -a,b
+c,d @@ syntax). Locate where display_hunk() is being called on the
BasicLineDiffPrinter instance and ensure that display_header() is invoked on the
printer before any display_hunk() calls to generate valid unified diff output
that can be applied as a patch.
In `@cpp-linter/src/rest_client.rs`:
- Around line 218-233: In summary-only mode, clang-tidy diagnostics without
fixes are not being added to review_comments before the decision logic runs,
causing review_comments.tool_total to be zero and allowing invalid PR approvals.
Before the block that determines options.action and options.summary (which uses
has_changes derived from review_comments.tool_total), ensure that no-fix
clang-tidy diagnostics are included in the review_comments concern count in
summary-only mode. This likely requires modifying make_suggestions_from_patch or
the code that populates review_comments to include no-fix diagnostics in the
tool_total count even when summary_only is true, so the action and summary
decisions reflect all tidy warnings.
🪄 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: 94621f1a-8c9c-4e79-9abb-b55f70c74f3d
📒 Files selected for processing (10)
cpp-linter/src/clang_tools/clang_format.rscpp-linter/src/clang_tools/clang_tidy.rscpp-linter/src/clang_tools/mod.rscpp-linter/src/cli/structs.rscpp-linter/src/common_fs.rscpp-linter/src/error.rscpp-linter/src/rest_client.rscpp-linter/src/run.rscpp-linter/tests/reviews.rsdocs/pyproject.toml
Comment on lines
+154
to
+185
| let mut cmd = Command::new(cmd_path); | ||
| cmd.current_dir(&cache_path); | ||
| // edit the clang-tody patched file in-place (`-i`) | ||
| cmd.args(["--style", &clang_params.style, "-i"]); | ||
| // if ranges is empty, then we're just formatting the entire file. | ||
| if !ranges.is_empty() { | ||
| // We're concerned about formatting what clang-tidy changed (tidy_diff.hunks().before), | ||
| // but we also want to include any clang-format changes that do not overlap clang-tidy fixes. | ||
| let mut joint_ranges = tidy_diff | ||
| .hunks() | ||
| // hunk is partially inclusive: [start, end), | ||
| // but clang-format expects fully inclusive line ranges. | ||
| // subtract 1 from hunk.before.end | ||
| .map(|hunk| { | ||
| RangeInclusive::new(hunk.before.start, hunk.before.end.saturating_sub(1)) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| for range in &ranges { | ||
| let mut contained = false; | ||
| for hunk in tidy_diff.hunks() { | ||
| if hunk.before.contains(range.start()) && hunk.before.contains(range.end()) { | ||
| contained = true; | ||
| break; | ||
| } | ||
| } | ||
| if !contained { | ||
| joint_ranges.push(range.clone()); | ||
| } | ||
| } | ||
| for range in &joint_ranges { | ||
| cmd.arg(format!("--lines={}:{}", range.start(), range.end()).as_str()); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full context of the file to understand the function
cd cpp-linter && head -n 200 src/clang_tools/clang_format.rs | tail -n 100Repository: cpp-linter/cpp-linter-rs
Length of output: 4401
🏁 Script executed:
# Let's see the complete function and understand what tidy_diff is
grep -n "tidy_diff" src/clang_tools/clang_format.rs | head -20Repository: cpp-linter/cpp-linter-rs
Length of output: 133
🏁 Script executed:
# Search for the make_patch function to understand tidy_diff structure
rg "make_patch" --type rust -A 5 -B 2Repository: cpp-linter/cpp-linter-rs
Length of output: 3857
🏁 Script executed:
# Look for imports to understand the diff library being used
head -50 src/clang_tools/clang_format.rsRepository: cpp-linter/cpp-linter-rs
Length of output: 159
🏁 Script executed:
# Search for how clang-format is actually invoked and what file is passed
rg "clang-format|--lines=" src/clang_tools/clang_format.rs -B 3 -A 3Repository: cpp-linter/cpp-linter-rs
Length of output: 140
🏁 Script executed:
# Find test files related to clang_format
find cpp-linter -name "*test*" -o -name "*tests*" | grep -i formatRepository: cpp-linter/cpp-linter-rs
Length of output: 50
🏁 Script executed:
# Search for tests involving tidy and format together
rg "tidy.*format|format.*tidy" cpp-linter --type rust -lRepository: cpp-linter/cpp-linter-rs
Length of output: 324
🏁 Script executed:
# Check the format_advice logic more carefully to see how it handles hunk.before/hunk.after
rg "is_pure_insertion|hunk.after|hunk.before" cpp-linter/src/clang_tools/clang_format.rs -B 2 -A 2Repository: cpp-linter/cpp-linter-rs
Length of output: 1074
🏁 Script executed:
# Look for InternedInput to understand the before/after semantics
rg "InternedInput" cpp-linter --type rust -B 3 -A 3Repository: cpp-linter/cpp-linter-rs
Length of output: 2190
🏁 Script executed:
# Check if there are any range translation utilities
rg "translate|convert.*range" cpp-linter/src/clang_tools/ --type rust -A 2Repository: cpp-linter/cpp-linter-rs
Length of output: 50
Use patched-file line ranges (hunk.after) when formatting the cached tidy output.
cmd.current_dir(&cache_path) formats the cached tidy-patched file, but joint_ranges is built from tidy_diff.hunks().before, which contains original-file coordinates. Since make_patch(&tidy_patch_contents, &original_contents) computes before as original-file lines and after as patched-file lines, using hunk.before targets the wrong lines. Tidy fixes that insert or delete lines shift line numbers in the patched file; this mismatch can cause clang-format to skip formatting or produce invalid ranges like --lines=10:9 for pure insertions.
Suggested direction
- let mut joint_ranges = tidy_diff
+ let mut joint_ranges = tidy_diff
.hunks()
- // hunk is partially inclusive: [start, end),
- // but clang-format expects fully inclusive line ranges.
- // subtract 1 from hunk.before.end
- .map(|hunk| {
- RangeInclusive::new(hunk.before.start, hunk.before.end.saturating_sub(1))
+ // clang-format is run against the cached patched file, so use
+ // patched-file coordinates and skip empty after-ranges.
+ .filter_map(|hunk| {
+ let end = hunk.after.end.checked_sub(1)?;
+ (hunk.after.start <= end).then(|| RangeInclusive::new(hunk.after.start, end))
})
.collect::<Vec<_>>();Also translate any non-overlapping original ranges before adding them to joint_ranges, so every --lines argument refers to the cached patched file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp-linter/src/clang_tools/clang_format.rs` around lines 154 - 185, The
joint_ranges are built using hunk.before which contains original-file line
coordinates, but the code is formatting the cached tidy-patched file where line
numbers have shifted due to tidy insertions/deletions. Replace hunk.before with
hunk.after in the map function that builds joint_ranges to use the correct
patched-file coordinates. Additionally, translate any non-overlapping original
ranges from the ranges parameter before adding them to joint_ranges, since they
also refer to original-file coordinates and must be converted to patched-file
coordinates before being passed as --lines arguments to clang-format.
Collaborator
Author
There was a problem hiding this comment.
This block is my main concern. I think I need to adjust the ranges (declared prior to this block) to use the line numbers in the patched file by clang-tidy. Because some of the insertions/deletions may alter the original line numbers, the ranges of original line numbers may be invalid.
Can you suggestion a function that abstracts this block, returns joint_ranges with the adjusted &ranges as compared to the &tidy_diff?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find make_patch definition
rg -n "pub fn make_patch" --type rust -A 10
# Also look for what Diff/UnifiedDiff type is imported/returned
rg -n "UnifiedDiff\|imara_diff\|make_patch" --type rust -B 2 -A 5Length of output: 5256
@2bndy5 Here's a proposed compute_format_ranges helper. The key idea:
- Part 1 — Every region clang-tidy changed, expressed in patched-file coordinates using
hunk.after(nothunk.before). Pure-deletion hunks (whereafter.start == after.end) produce nothing for clang-format. - Part 2 — Each original
rangethat is not fully contained inside a tidy hunk, translated to patched-file coordinates by accumulating the net line-count shift from all preceding tidy hunks.
/// Compute the `--lines` ranges (in **patched-file coordinates**) to pass to
/// `clang-format -i` when formatting the clang-tidy-patched file.
///
/// # Arguments
/// * `tidy_diff` – diff computed as `make_patch(&tidy_patched, &original)`, so
/// `hunk.before` = original-file range (exclusive end),
/// `hunk.after` = patched-file range (exclusive end).
/// * `ranges` – original-file line ranges from `--lines-changed-only`.
/// **Caller must only invoke this when `ranges` is non-empty.**
///
/// Returns a vector containing:
/// 1. Every region clang-tidy changed, in patched-file coordinates.
/// 2. Every region from `ranges` that does **not** overlap a clang-tidy hunk,
/// translated to patched-file coordinates.
fn compute_format_ranges(tidy_diff: &Diff, ranges: &[RangeInclusive<u32>]) -> Vec<RangeInclusive<u32>> {
let tidy_hunks: Vec<_> = tidy_diff.hunks().collect();
// Part 1 – tidy-changed regions in patched-file coordinates.
// hunk.after is [start, end); convert to fully-inclusive for clang-format.
// Skip pure-deletion hunks where after.start == after.end (nothing to format).
let mut joint_ranges: Vec<RangeInclusive<u32>> = tidy_hunks
.iter()
.filter_map(|hunk| {
let end = hunk.after.end.saturating_sub(1);
(hunk.after.start <= end).then(|| RangeInclusive::new(hunk.after.start, end))
})
.collect();
// Part 2 – original ranges not covered by tidy, translated to patched coords.
for range in ranges {
// Skip ranges fully contained within a tidy hunk's before-range;
// they are already covered by Part 1 above.
let contained = tidy_hunks.iter().any(|hunk| {
hunk.before.contains(range.start()) && hunk.before.contains(range.end())
});
if contained {
continue;
}
// Translate by summing the net line-count change of every tidy hunk
// that ends strictly before this range's start.
// hunk.before / hunk.after are [start, end) so before.end is exclusive.
let mut shift: i64 = 0;
for hunk in &tidy_hunks {
if hunk.before.end <= *range.start() {
let before_len = hunk.before.end as i64 - hunk.before.start as i64;
let after_len = hunk.after.end as i64 - hunk.after.start as i64;
shift += after_len - before_len;
}
}
let translated_start = (*range.start() as i64 + shift) as u32;
let translated_end = (*range.end() as i64 + shift) as u32;
joint_ranges.push(RangeInclusive::new(translated_start, translated_end));
}
joint_ranges
}The call-site then becomes:
if !ranges.is_empty() {
for range in compute_format_ranges(&tidy_diff, &ranges) {
cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
}
}A couple of notes:
- The
shiftaccumulation intentionally only counts hunks whosebefore.end <= range.start()— i.e., hunks completely before the range. Since we already skipped ranges overlapping a tidy hunk, the shift atrange.startandrange.endis the same (they sit in the same inter-hunk gap), so one shift value suffices for both endpoints. - If you want to cover ranges that are partially overlapping a tidy hunk (currently silently dropped), that would need an extra split/merge step — but that's probably an edge case worth a follow-up issue rather than bloating this PR.
🧬 Code Graph Analysis Results
cpp-linter/src/clang_tools/clang_format.rs
Joint-range construction block (the part your comment is concerned with)
Lines 159-186
// if ranges is empty, then we're just formatting the entire file.
if !ranges.is_empty() {
// We're concerned about formatting what clang-tidy changed (tidy_diff.hunks().before),
// but we also want to include any clang-format changes that do not overlap clang-tidy fixes.
let mut joint_ranges = tidy_diff
.hunks()
// hunk is partially inclusive: [start, end),
// but clang-format expects fully inclusive line ranges.
// subtract 1 from hunk.before.end
.map(|hunk| {
RangeInclusive::new(hunk.before.start, hunk.before.end.saturating_sub(1))
})
.collect::<Vec<_>>();
for range in &ranges {
let mut contained = false;
for hunk in tidy_diff.hunks() {
if hunk.before.contains(range.start()) && hunk.before.contains(range.end()) {
contained = true;
break;
}
}
if !contained {
joint_ranges.push(range.clone());
}
}
for range in &joint_ranges {
cmd.arg(format!("--lines={}:{}", range.start(), range.end()).as_str());
}
}Where ranges comes from
Lines 67-70
let ranges = file.get_ranges(&clang_params.lines_changed_only);
for range in &ranges {
cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
}Where tidy_diff comes from
Lines 153-158
let (tidy_diff, _) = make_patch(&tidy_patch_contents, &original_contents);
let mut cmd = Command::new(cmd_path);
cmd.current_dir(&cache_path);
// edit the clang-tody patched file in-place (`-i`)
cmd.args(["--style", &clang_params.style, "-i"]);cpp-linter/src/common_fs.rs
FileObj fields that ultimately determine what get_ranges() returns
Lines 25-46
pub struct FileObj {
/// The path to the file.
pub name: PathBuf,
/// The list of lines with additions.
pub added_lines: Vec<u32>,
/// The list of ranges that span only lines with additions.
pub added_ranges: Vec<RangeInclusive<u32>>,
/// The list of ranges that span the lines present in diff chunks.
pub diff_chunks: Vec<RangeInclusive<u32>>,
/// The collection of clang-format advice for this file.
pub format_advice: Option<FormatAdvice>,
/// The collection of clang-format advice for this file.
pub tidy_advice: Option<TidyAdvice>,
/// A path to a cached file with all/any patches applied.
pub(crate) patched_path: Option<PathBuf>,
}get_ranges() implementation (used to produce ranges in clang_format.rs)
Lines 48-324 (relevant excerpt: get_ranges)
/// Get the list of line ranges to consider based on the given
/// [`LinesChangedOnly`] configuration.
pub fn get_ranges(&self, lines_changed_only: &LinesChangedOnly) -> Vec<RangeInclusive<u32>> {
match lines_changed_only {
LinesChangedOnly::Diff => self.diff_chunks.to_vec(),
LinesChangedOnly::On => self.added_ranges.to_vec(),
_ => Vec::new(),
}
}cpp-linter/src/clang_tools/mod.rs
Patch/diff creation used to compute tidy_diff and the formatting diff hunks
Lines 328-336
pub fn make_patch<'buffer>(
patched: &'buffer str,
original_content: &'buffer str,
) -> (Diff, InternedInput<&'buffer str>) {
let input = InternedInput::new(original_content, patched);
let mut diff = Diff::compute(gix_imara_diff::Algorithm::Histogram, &input);
diff.postprocess_lines(&input);
(diff, input)
}🧠 Learnings used
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/version.rs:301-304
Timestamp: 2026-03-30T06:22:32.553Z
Learning: In this repo’s Rust tests (run via `cargo-nextest` configured by `.config/nextest.toml`), each test runs in its own process. Therefore, `unsafe { std::env::set_var(...) }` used inside tests is safe with respect to cross-test environment variable pollution, and you generally do not need to implement a save-and-restore guard for env vars in test code.
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: cpp-linter/src/run.rs:6-6
Timestamp: 2026-03-30T06:29:55.617Z
Learning: When reviewing Rust code in this repo, don’t assume `#![cfg(feature = "bin")]` feature-gated modules are excluded from integration test execution. The project’s test runner (`nur test`, from `nurfile`) always invokes `cargo llvm-cov nextest` with `--features bin`, so code gated by the `bin` feature will be compiled during tests. Only flag test-coverage/test-discovery concerns if the gating is not controlled by the `bin` feature (or if the test invocation in CI is changed to not pass `--features bin`).
Collaborator
Author
There was a problem hiding this comment.
If you want to cover ranges that are partially overlapping a tidy hunk (currently silently dropped), that would need an extra split/merge step — but that's probably an edge case worth a follow-up issue rather than bloating this PR.
That's not an edge case. Its the complexity that I was implicating. I'm not dismissing the proposal, rather I was expecting more (thus the new function to abstract the complexity).
Contributor
There was a problem hiding this comment.
@2bndy5, fair point — partial overlap is the real scenario to handle. Here's an updated compute_format_ranges that fully handles it by splitting each range around every overlapping tidy hunk, dropping the covered fragments, and translating the survivors to patched-file coordinates:
/// Compute the `--lines` ranges (in **patched-file coordinates**) to pass to
/// `clang-format -i` when formatting the clang-tidy-patched file.
///
/// # Arguments
/// * `tidy_diff` – diff computed as `make_patch(&tidy_patched, &original)`, so:
/// - `hunk.before` = original-file range `[start, end)` (exclusive end)
/// - `hunk.after` = patched-file range `[start, end)` (exclusive end)
/// * `ranges` – original-file line ranges from `--lines-changed-only`.
/// **Caller is responsible for only calling this when `ranges` is non-empty.**
///
/// Returns the minimal set of fully-inclusive line ranges for the patched file,
/// covering:
/// 1. Every region clang-tidy changed (`hunk.after`).
/// 2. Every fragment of `ranges` that does NOT overlap a tidy hunk,
/// translated to patched-file coordinates.
fn compute_format_ranges(tidy_diff: &Diff, ranges: &[RangeInclusive<u32>]) -> Vec<RangeInclusive<u32>> {
let tidy_hunks: Vec<_> = tidy_diff.hunks().collect();
// Part 1 – tidy-changed regions in patched-file (after) coordinates.
// hunk.after is [start, end) exclusive; convert to fully-inclusive.
// Pure-deletion hunks (after.start == after.end) produce nothing to format.
let mut joint_ranges: Vec<RangeInclusive<u32>> = tidy_hunks
.iter()
.filter_map(|hunk| {
let end = hunk.after.end.checked_sub(1)?;
(hunk.after.start <= end).then(|| RangeInclusive::new(hunk.after.start, end))
})
.collect();
// Part 2 – fragments of each original `range` that lie entirely in the
// gaps *between* tidy hunks, translated to patched-file coordinates.
//
// Algorithm:
// For each range, progressively subtract every tidy hunk's before-range
// [hunk.before.start, hunk.before.end), keeping only the leftover
// sub-segments. Then translate each surviving sub-segment by the
// cumulative net-line-count change of all preceding tidy hunks.
for range in ranges {
// Work with inclusive coordinates throughout.
let mut remaining: Vec<RangeInclusive<u32>> = vec![range.clone()];
for hunk in &tidy_hunks {
// hunk.before = [start, end) in original coords.
let hunk_start = hunk.before.start;
let hunk_end_excl = hunk.before.end; // exclusive
let mut survivors = Vec::new();
for seg in remaining {
// Fragment of `seg` strictly before this hunk.
if *seg.start() < hunk_start {
let end = (*seg.end()).min(hunk_start.saturating_sub(1));
survivors.push(RangeInclusive::new(*seg.start(), end));
}
// Fragment of `seg` strictly after this hunk.
if *seg.end() >= hunk_end_excl {
let start = (*seg.start()).max(hunk_end_excl);
survivors.push(RangeInclusive::new(start, *seg.end()));
}
// The overlapping fragment is intentionally dropped — it is
// already covered by Part 1 (hunk.after).
}
remaining = survivors;
}
// Translate each surviving sub-segment to patched-file coordinates.
// The shift at a point P is the sum of (after_len - before_len) for
// every tidy hunk whose before-range ends strictly before P.
// Because each sub-segment lies entirely within a single inter-hunk
// gap, both its start and end have the same shift.
for seg in remaining {
let mut shift: i64 = 0;
for hunk in &tidy_hunks {
if hunk.before.end <= *seg.start() {
let before_len = hunk.before.end as i64 - hunk.before.start as i64;
let after_len = hunk.after.end as i64 - hunk.after.start as i64;
shift += after_len - before_len;
}
}
let translated_start = (*seg.start() as i64 + shift) as u32;
let translated_end = (*seg.end() as i64 + shift) as u32;
joint_ranges.push(RangeInclusive::new(translated_start, translated_end));
}
}
joint_ranges
}The call-site simplifies to:
if !ranges.is_empty() {
for range in compute_format_ranges(&tidy_diff, &ranges) {
cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
}
}How the three cases are handled:
| Case | Behaviour |
|---|---|
Range entirely inside a tidy hunk's before |
Splits leave nothing; already covered by Part 1 |
| Range with no overlap with any tidy hunk | No splits; translated by cumulative shift of preceding hunks |
| Range partially overlapping one or more hunks | Split into fragments; overlapping fragments dropped (covered by Part 1), outer fragments translated by appropriate shift |
The shift accumulation is safe because each surviving sub-segment sits in a gap between two adjacent hunks (or before the first / after the last), so every point within it has the same cumulative shift.
🐇 ～(__)
🧠 Learnings used
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/version.rs:301-304
Timestamp: 2026-03-30T06:22:32.553Z
Learning: In this repo’s Rust tests (run via `cargo-nextest` configured by `.config/nextest.toml`), each test runs in its own process. Therefore, `unsafe { std::env::set_var(...) }` used inside tests is safe with respect to cross-test environment variable pollution, and you generally do not need to implement a save-and-restore guard for env vars in test code.
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: cpp-linter/src/run.rs:6-6
Timestamp: 2026-03-30T06:29:55.617Z
Learning: When reviewing Rust code in this repo, don’t assume `#![cfg(feature = "bin")]` feature-gated modules are excluded from integration test execution. The project’s test runner (`nur test`, from `nurfile`) always invokes `cargo llvm-cov nextest` with `--features bin`, so code gated by the `bin` feature will be compiled during tests. Only flag test-coverage/test-discovery concerns if the gating is not controlled by the `bin` feature (or if the test invocation in CI is changed to not pass `--features bin`).
Comment on lines
+261
to
+263
| let mut config = UnifiedDiffConfig::default(); | ||
| config.context_len(0); | ||
| let printer = BasicLineDiffPrinter(&input.interner); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In gix-imara-diff 0.2.2, does UnifiedDiffPrinter::display_hunk() include @@ hunk headers, or must display_header() be called separately to produce unified-diff-compliant output?
💡 Result:
In gix-imara-diff 0.2.2, UnifiedDiffPrinter::display_hunk does NOT include the unified-diff hunk header line (the “@@ -a,b +c,d @@” line). For unified-diff-compliant output, display_header must be called separately before display_hunk. [1][2] Evidence: the trait defines three separate required formatting hooks—display_header, display_context_token, and display_hunk—with display_header taking the before/after start and lengths needed to construct the “@@ -… +…” header, while display_hunk only receives the token slices for the before/after ranges (not the header metadata). [1] Additionally, gix-imara-diff’s built-in unified diff output (shown in the crate docs example) has a header line starting with “@@ … @@” before the hunk’s changed lines, consistent with header emission separate from hunk body emission. [3] So: call display_header to emit the “@@ … @@” line, then call display_hunk to emit the context/add/remove lines for that hunk.
Citations:
- 1: https://docs.rs/imara-diff/latest/imara_diff/trait.UnifiedDiffPrinter.html
- 2: https://docs.rs/imara-diff/latest/imara_diff/struct.BasicLineDiffPrinter.html
- 3: https://docs.rs/gix-imara-diff/latest/gix_imara_diff/
Call display_header() before display_hunk() to generate valid unified diff hunks.
In gix-imara-diff 0.2.2, the UnifiedDiffPrinter::display_hunk() method does not emit the hunk header line (@@ -a,b +c,d @@). The display_header() method must be called separately before display_hunk() to produce unified-diff-compliant output. Without the header, ReviewComments.full_patch becomes an unapplyable patch fragment.
Suggested fix
- printer.display_hunk(
+ printer.display_header(
+ &mut patch_buff,
+ hunk.before.start,
+ hunk.after.start,
+ hunk.before.len() as u32,
+ hunk.after.len() as u32,
+ )?;
+ printer.display_hunk(
&mut patch_buff,
&input.before[hunk.before.start as usize..hunk.before.end as usize],
&input.after[hunk.after.start as usize..hunk.after.end as usize],
)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp-linter/src/common_fs.rs` around lines 261 - 263, The UnifiedDiffPrinter's
display_hunk() method requires that display_header() be called first to emit the
hunk header line (the @@ -a,b +c,d @@ syntax). Locate where display_hunk() is
being called on the BasicLineDiffPrinter instance and ensure that
display_header() is invoked on the printer before any display_hunk() calls to
generate valid unified diff output that can be applied as a patch.
Source: MCP tools
Comment on lines
+218
to
+233
| let total_review_comments = options.comments.len() as u32; | ||
| self.client.cull_pr_reviews(&mut options).await?; | ||
| let has_changes = review_comments.full_patch.iter().any(|p| !p.is_empty()); | ||
| let has_changes = review_comments.tool_total > 0; | ||
| options.action = if feedback_inputs.passive_reviews { | ||
| ReviewAction::Comment | ||
| } else if options.comments.is_empty() && !has_changes { | ||
| ReviewAction::Approve | ||
| } else { | ||
| ReviewAction::RequestChanges | ||
| }; | ||
| options.summary = review_comments.summarize(&clang_versions, &options.comments); | ||
| options.summary = review_comments.summarize( | ||
| &clang_versions, | ||
| &options.comments, | ||
| total_review_comments, | ||
| summary_only, | ||
| ); |
Contributor
There was a problem hiding this comment.
Don’t let summary-only reviews drop no-fix clang-tidy diagnostics.
With CPP_LINTER_PR_REVIEW_SUMMARY_ONLY=1, make_suggestions_from_patch returns before adding clang-tidy diagnostics that have no fixes. Since this block now derives both options.action and options.summary from review_comments.tool_total, a PR with only non-fixable tidy warnings can be approved and summarized as clean.
Fix this by ensuring no-fix tidy diagnostics still contribute to the review concern count in summary-only mode before this action/summary decision is made.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp-linter/src/rest_client.rs` around lines 218 - 233, In summary-only mode,
clang-tidy diagnostics without fixes are not being added to review_comments
before the decision logic runs, causing review_comments.tool_total to be zero
and allowing invalid PR approvals. Before the block that determines
options.action and options.summary (which uses has_changes derived from
review_comments.tool_total), ensure that no-fix clang-tidy diagnostics are
included in the review_comments concern count in summary-only mode. This likely
requires modifying make_suggestions_from_patch or the code that populates
review_comments to include no-fix diagnostics in the tool_total count even when
summary_only is true, so the action and summary decisions reflect all tidy
warnings.
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
1 participant
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.
This is a culmination of
As this builds on the plan discussed in cpp-linter/cpp-linter#82, it it the last step needed before providing a patch that consumers can push to auto-fix lints.
Intended flow of data
--lines-changed-onlyis respected, but this may need tweaking in the future.CPP_LINTER_PR_REVIEW_SUMMARY_ONLY = 1. If PR review is a summary only, then the patch will include all fixes that would have been posted in the diff.Adjusted tests accordingly.
Summary by CodeRabbit
Bug Fixes
Chores
Tests