From 57c9a91e469873681894bbcc03a04fd94af0beed Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Mon, 19 Oct 2026 00:48:46 +0000 Subject: [PATCH 1/4] feat: report clang-tidy fixes that could not be applied to other files A fix for another file that is not applied (eg. the header is not analyzed) is recorded on the diagnostic's note. The thread comment, the PR review's suggestions and the review summary now say which file was left out and why, so users can apply that part themselves. --- cpp-linter/src/clang_tools/clang_tidy/mod.rs | 30 +++++++++ .../clang_tools/clang_tidy/other_files/mod.rs | 65 ++++++++++++++----- .../clang_tidy/other_files/test.rs | 40 +++++++++--- cpp-linter/src/clang_tools/clang_tidy/test.rs | 23 +++++++ cpp-linter/src/clang_tools/mod.rs | 27 ++++++++ cpp-linter/src/common_fs.rs | 36 +++++++++- cpp-linter/src/rest_client.rs | 38 ++++++++++- cpp-linter/src/sarif.rs | 1 + docs/docs/pr-review-caveats.md | 6 ++ 9 files changed, 235 insertions(+), 31 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_tidy/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/mod.rs index e90c560a..a245e6e4 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/mod.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/mod.rs @@ -70,6 +70,32 @@ pub struct TidyNotification { /// The list of line numbers (in the analyzed file) that clang-tidy suggested fixes for. pub fixed_lines: Vec, + + /// The parts of the diagnostic's fix that were not applied to other files + /// (eg. a header that is not one of the analyzed files). + /// + /// Users have to apply these parts themselves, so they are mentioned in the + /// thread comment and PR review. + pub skipped_fixes: Vec, +} + +/// A part of a clang-tidy fix that was not applied to another file. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SkippedFix { + /// The other file's path (relative to the repository root, if possible). + pub file: String, + /// Why the fix was not applied to the other file. + pub reason: String, +} + +impl SkippedFix { + /// Describe the skipped fix in a sentence (in markdown). + pub fn describe(&self) -> String { + format!( + "Part of this fix was not applied to `{}` because {}.", + self.file, self.reason + ) + } } /// The URL to clang-tidy's documentation. @@ -135,6 +161,9 @@ impl TidyAdvice { note.rationale, note.diagnostic_link() )); + for skipped in ¬e.skipped_fixes { + diagnostics.push(format!(" - :warning: {}\n", skipped.describe())); + } } } } @@ -232,6 +261,7 @@ fn parse_tidy_output( diagnostic: String::from(&captured[6]), suggestion: Vec::new(), fixed_lines: Vec::new(), + skipped_fixes: vec![], }); } } else if let Some(note) = &mut notification { diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs index 8bd3a17d..da194c6a 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/mod.rs @@ -16,7 +16,8 @@ use std::{ use clang_tools_manager::utils::normalize_path; use super::{ - ExportedReplacement, TidyAdvice, TidyNotification, offset_to_line, patch_content, write_cache, + ExportedReplacement, SkippedFix, TidyAdvice, TidyNotification, offset_to_line, patch_content, + write_cache, }; use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError}; @@ -262,23 +263,25 @@ pub fn apply_other_file_fixes( .add(fix, line) }), }; - logs[*source_index].push(match verdict.err() { - None => ( - log::Level::Info, - format!( - "Applying the fix for {} (reported in {source_name}) to {other}", - fix.diagnostic, - ), + let message = match verdict { + Ok(()) => format!( + "Applying the fix for {} (reported in {source_name}) to {other}", + fix.diagnostic, ), - Some(rejection) => ( - log::Level::Info, - format!( - "Not applying the fix for {} (reported in {source_name}) to {other} because {}", - fix.diagnostic, - rejection.reason(), - ), - ), - }); + Err(rejection) => { + let skipped = SkippedFix { + file: other.replace('\\', "/"), + reason: rejection.reason(), + }; + let message = format!( + "Not applying the fix for {} (reported in {source_name}) to {} because {}", + fix.diagnostic, skipped.file, skipped.reason, + ); + record_skipped_fix(&files[*source_index], fix, skipped)?; + message + } + }; + logs[*source_index].push((log::Level::Info, message)); } } for ( @@ -324,6 +327,33 @@ fn overlaps(a: &Replacement, b: &Replacement) -> bool { first != second && second.offset < first.offset + first.length } +/// Record a `skipped` part of a `fix` on the note about the fix's diagnostic in the +/// analyzed `source` file, so the thread comment and PR review can mention it. +/// +/// Nothing is recorded if no note about the diagnostic was found. +fn record_skipped_fix( + source: &Arc>, + fix: &OtherFileFix, + skipped: SkippedFix, +) -> Result<(), ClangCaptureError> { + let Some(fix_note) = &fix.note else { + return Ok(()); + }; + let mut source = source + .lock() + .map_err(|_| ClangCaptureError::MutexPoisoned)?; + let note = source + .tidy_advice + .as_mut() + .and_then(|advice| advice.notes.iter_mut().find(|n| is_same_note(n, fix_note))); + if let Some(note) = note + && !note.skipped_fixes.contains(&skipped) + { + note.skipped_fixes.push(skipped); + } + Ok(()) +} + /// The accepted fixes for one analyzed file. struct Pending { /// The file's own fixes, followed by the accepted fixes from other files. @@ -357,6 +387,7 @@ impl Pending { }; let note = TidyNotification { fixed_lines: vec![line], + skipped_fixes: vec![], ..note.clone() }; if let Some(existing) = self.notes.iter_mut().find(|n| is_same_note(n, ¬e)) { diff --git a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs index 287a322c..d25c86b7 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/other_files/test.rs @@ -14,7 +14,7 @@ use super::{OtherFileFix, Pending, Replacement, TidyFixes, apply_other_file_fixe use crate::{ clang_tools::{ ClangTool, analyze_files, - clang_tidy::{TidyNotification, tally_tidy_advice}, + clang_tidy::{SkippedFix, TidyAdvice, TidyNotification, tally_tidy_advice}, }, cli::{ClangParams, LinesChangedOnly}, common_fs::FileObj, @@ -74,6 +74,7 @@ fn source_note() -> TidyNotification { diagnostic: "readability-non-const-parameter".to_string(), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], } } @@ -171,20 +172,39 @@ fn not_analyzed() { repo_root: tmp.path().to_path_buf(), ..Default::default() }; - let files = as_files(vec![FileObj::new(PathBuf::from("fix.cpp"))]); + let mut source = FileObj::new(PathBuf::from("fix.cpp")); + source.tidy_advice = Some(TidyAdvice { + notes: vec![source_note()], + }); + let files = as_files(vec![source]); + let mut without_note = header_fix(tmp.path()); + without_note.note = None; + let fixes = TidyFixes { + own: vec![], + // the same fix twice, and a fix whose diagnostic has no note + others: vec![header_fix(tmp.path()), header_fix(tmp.path()), without_note], + }; let mut logs = vec![vec![]]; - apply_other_file_fixes( - &files, - &[(0, header_fixes(tmp.path()))], - &clang_params, - &mut logs, - ) - .unwrap(); + apply_other_file_fixes(&files, &[(0, fixes)], &clang_params, &mut logs).unwrap(); assert!(patched(&files[0]).is_none()); let messages = messages(&logs[0]); - assert_eq!(messages.len(), 1); + assert_eq!(messages.len(), 3); assert!(messages[0].starts_with("Not applying the fix for readability-non-const-parameter")); assert!(messages[0].ends_with("to fix.h because it is not one of the analyzed files")); + // the skipped fix is recorded once on the diagnostic's note + let file = files[0].lock().unwrap(); + let note = &file.tidy_advice.as_ref().unwrap().notes[0]; + assert_eq!( + note.skipped_fixes, + vec![SkippedFix { + file: "fix.h".to_string(), + reason: "it is not one of the analyzed files".to_string(), + }] + ); + assert_eq!( + note.skipped_fixes[0].describe(), + "Part of this fix was not applied to `fix.h` because it is not one of the analyzed files." + ); } #[test] diff --git a/cpp-linter/src/clang_tools/clang_tidy/test.rs b/cpp-linter/src/clang_tools/clang_tidy/test.rs index d5800090..ebcaf0ac 100644 --- a/cpp-linter/src/clang_tools/clang_tidy/test.rs +++ b/cpp-linter/src/clang_tools/clang_tidy/test.rs @@ -32,6 +32,7 @@ fn clang_diagnostic_link() { diagnostic: String::from("clang-diagnostic-error"), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], }; assert_eq!(note.diagnostic_link(), note.diagnostic); } @@ -47,6 +48,7 @@ fn clang_diagnostic_warning_link() { diagnostic: String::from("clang-diagnostic-unused-variable"), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], }; let expected = format!( "[{}]({CLANG_DIAGNOSTICS_URL}#w{})", @@ -66,6 +68,7 @@ fn clang_analyzer_link() { diagnostic: String::from("clang-analyzer-core.NullDereference"), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], }; let expected = format!( "[{}](https://clang.llvm.org/extra/clang-tidy/checks/{}/{}.html)", @@ -86,6 +89,7 @@ fn invalid_diagnostic_link() { diagnostic: expected.clone(), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], }; assert_eq!(note.diagnostic_link(), expected); } @@ -228,6 +232,7 @@ fn dummy_note(filename: &str, fixed_lines: Vec) -> TidyNotification { diagnostic: String::from("modernize-use-auto"), suggestion: vec![], fixed_lines, + skipped_fixes: vec![], } } @@ -264,6 +269,23 @@ fn suggestion_help_with_and_without_fixes() { assert!(help.contains("clang-tidy suggestion")); } +#[test] +fn suggestion_help_with_skipped_fixes() { + use super::{SkippedFix, TidyAdvice}; + + let mut note = dummy_note("demo.cpp", vec![5]); + note.skipped_fixes.push(SkippedFix { + file: "demo.h".to_string(), + reason: "it is not one of the analyzed files".to_string(), + }); + let advice = TidyAdvice { notes: vec![note] }; + let help = advice.get_suggestion_help(5, 5); + assert!(help.contains( + "\n - :warning: Part of this fix was not applied to `demo.h` \ + because it is not one of the analyzed files.\n" + )); +} + #[test] fn parse_non_utf8_output() { let err = parse_tidy_output(&[0xFF, 0xFE], &None, &PathBuf::from(".")).unwrap_err(); @@ -528,6 +550,7 @@ fn apply_exported_fixes() { // the matching note, to explain the fix once it is applied note: Some(TidyNotification { fixed_lines: vec![], + skipped_fixes: vec![], ..notes[0].clone() }), }] diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 305f99df..d2ae8feb 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -273,6 +273,9 @@ pub struct ReviewComments { /// This includes changes from both clang-tidy and clang-format /// (assembled in that order). pub full_patch: String, + /// Markdown list items about the parts of clang-tidy fixes that were not applied + /// to other files (see [`clang_tidy::TidyNotification::skipped_fixes`]). + pub skipped_fixes: Vec, } impl ReviewComments { @@ -335,6 +338,14 @@ impl ReviewComments { format!("\n{dupes} suggestions were duplicates of previous reviews.\n").as_str(), ); } + if !self.skipped_fixes.is_empty() { + body.push_str( + "\nSome clang-tidy fixes also change files that cpp-linter did not change. \ + Please apply these parts yourself:\n\n", + ); + body.push_str(&self.skipped_fixes.join("\n")); + body.push('\n'); + } // The `full_patch` includes all suggestions that didn't fit in the diff. // It can also contain suggestions that were duplicates of previous reviews. if !self.full_patch.is_empty() { @@ -474,6 +485,22 @@ mod tests { assert!(review_summary.contains("suggestions were duplicates of previous reviews")); } + #[test] + fn summarize_skipped_fixes() { + let clang_versions = ClangVersions { + format_version: None, + tidy_version: Some(PSEUDO_VERSION), + }; + let review_summary = ReviewComments { + skipped_fixes: vec!["- first".to_string(), "- second".to_string()], + ..Default::default() + } + .summarize(&clang_versions, &[], 0, true); + assert!( + review_summary.contains("Please apply these parts yourself:\n\n- first\n- second\n") + ); + } + #[test] fn summary_len_truncated() { let comments = vec![ReviewComment { diff --git a/cpp-linter/src/common_fs.rs b/cpp-linter/src/common_fs.rs index f8d82347..d58e409b 100644 --- a/cpp-linter/src/common_fs.rs +++ b/cpp-linter/src/common_fs.rs @@ -250,6 +250,16 @@ impl FileObj { // Count of clang-tidy diagnostics that had no fixes applied let mut total = 0; for note in &advice.notes { + if note.filename == file_name { + for skipped in ¬e.skipped_fixes { + review_comments.skipped_fixes.push(format!( + "- **{file_name}:{}** [{}]: {}", + note.line, + note.diagnostic_link(), + skipped.describe() + )); + } + } // Skip diagnostics about other files (eg. included headers); // their line numbers do not refer to this file. if note.filename == file_name @@ -275,6 +285,9 @@ impl FileObj { .as_str(), ); } + for skipped in ¬e.skipped_fixes { + suggestion.push_str(&format!("\n:warning: {}\n", skipped.describe())); + } let mut is_merged = false; for s in &mut review_comments.comments { if s.path == file_name @@ -506,7 +519,7 @@ mod test { #[test] fn unfixed_tidy_diagnostics() { - use crate::clang_tools::clang_tidy::{TidyAdvice, TidyNotification}; + use crate::clang_tools::clang_tidy::{SkippedFix, TidyAdvice, TidyNotification}; let repo_root = TempDir::new().unwrap(); let file_name = PathBuf::from("test_file.cpp"); @@ -527,6 +540,7 @@ mod test { diagnostic: "readability-identifier-length".to_string(), suggestion, fixed_lines: vec![], + skipped_fixes: vec![], }; let mut file_obj = FileObj::from(file_name, vec![1, 3], vec![1..=1, 3..=4]); file_obj.patched_path = Some(patched_file.path().to_path_buf()); @@ -535,7 +549,13 @@ mod test { // merged into the existing suggestion for line 1 note(1, vec![]), // not in any existing suggestion; creates a new suggestion - note(3, vec!["line3".to_string()]), + TidyNotification { + skipped_fixes: vec![SkippedFix { + file: "test_file.h".to_string(), + reason: "it is not one of the analyzed files".to_string(), + }], + ..note(3, vec!["line3".to_string()]) + }, // a diagnostic about an included header is not about this file's line 4 TidyNotification { filename: "test_file.h".to_string(), @@ -559,6 +579,18 @@ mod test { assert_eq!(new_suggestion.line_end, 3); assert!(new_suggestion.suggestion.contains("rationale for line 3")); assert!(new_suggestion.suggestion.contains("```cpp\nline3\n```")); + let skipped = "Part of this fix was not applied to `test_file.h` \ + because it is not one of the analyzed files."; + assert!( + new_suggestion + .suggestion + .contains(&format!("\n:warning: {skipped}\n")) + ); + assert_eq!(review_comments.skipped_fixes.len(), 1); + assert!( + review_comments.skipped_fixes[0].starts_with("- **test_file.cpp:3** [[readability") + ); + assert!(review_comments.skipped_fixes[0].ends_with(skipped)); // Processing the same file again merges duplicate suggestions file_obj diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 551a0ebc..8608d15f 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -554,7 +554,7 @@ fn make_tidy_comment( let uses_auto_fix = !tidy_note.fixed_lines.is_empty(); let mut tmp_note = format!("- {}\n\n", tidy_note.filename); tmp_note.push_str(&format!( - " {filename}:{line}:{cols}: {severity}: [{diagnostic}]{auto_fixable}\n > {rationale}\n{concerned_code}", + " {filename}:{line}:{cols}: {severity}: [{diagnostic}]{auto_fixable}{skipped_fixes}\n > {rationale}\n{concerned_code}", filename = tidy_note.filename, line = tidy_note.line, cols = tidy_note.cols, @@ -565,6 +565,11 @@ fn make_tidy_comment( } else { "" }, + skipped_fixes = tidy_note + .skipped_fixes + .iter() + .map(|skipped| format!("\n :warning: {}", skipped.describe())) + .collect::(), rationale = tidy_note.rationale, concerned_code = if tidy_note.suggestion.is_empty() {String::from("")} else { format!("\n ```{ext}\n {suggestion}\n ```\n", @@ -625,7 +630,7 @@ mod test { clang_tools::{ ClangVersions, clang_format::FormatAdvice, - clang_tidy::{TidyAdvice, TidyNotification}, + clang_tidy::{SkippedFix, TidyAdvice, TidyNotification}, }, cli::{FeedbackInput, ThreadComments}, common_fs::FileObj, @@ -664,6 +669,7 @@ mod test { diagnostic: String::from("clang-diagnostic-warning"), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], }]; file.tidy_advice = Some(TidyAdvice { notes }); file.format_advice = Some(FormatAdvice { @@ -755,6 +761,7 @@ mod test { diagnostic: String::from("clang-diagnostic-error"), suggestion: vec![String::from("int main() {}")], fixed_lines: vec![1], + skipped_fixes: vec![], }], }); file.format_advice = Some(FormatAdvice { @@ -764,6 +771,33 @@ mod test { vec![Arc::new(Mutex::new(file))] } + #[test] + fn tidy_comment_with_skipped_fixes() { + let files = make_dummy_files("warning"); + { + let mut file = files[0].lock().unwrap(); + let note = &mut file.tidy_advice.as_mut().unwrap().notes[0]; + note.skipped_fixes.push(SkippedFix { + file: "tests/demo/demo.hpp".to_string(), + reason: "it is not one of the analyzed files".to_string(), + }); + } + let mut comment = String::new(); + let mut remaining_length = u64::from(u16::MAX); + super::make_tidy_comment( + &files, + &mut comment, + 1, + &"1.2.3".to_string(), + &mut remaining_length, + ) + .unwrap(); + assert!(comment.contains( + "auto-fix included in generated patch[^1]\n :warning: Part of this fix was not \ + applied to `tests/demo/demo.hpp` because it is not one of the analyzed files.\n > " + )); + } + fn dummy_versions() -> ClangVersions { ClangVersions { format_version: Some(Version::new(1, 2, 3)), diff --git a/cpp-linter/src/sarif.rs b/cpp-linter/src/sarif.rs index 8ded7444..05367d3d 100644 --- a/cpp-linter/src/sarif.rs +++ b/cpp-linter/src/sarif.rs @@ -356,6 +356,7 @@ mod test { diagnostic: diagnostic.to_string(), suggestion: vec![], fixed_lines: vec![], + skipped_fixes: vec![], } } diff --git a/docs/docs/pr-review-caveats.md b/docs/docs/pr-review-caveats.md index ddc1c123..84dbee49 100644 --- a/docs/docs/pr-review-caveats.md +++ b/docs/docs/pr-review-caveats.md @@ -84,3 +84,9 @@ Request diff (the summation of file differences in a Pull Request). - Some fixes provided might depend on what compiler is used. We have made it so clang-tidy takes advantage of any fixes provided by the compiler. Compilation errors may still prevent clang-tidy from reporting all concerns. +- A clang-tidy fix can also change another file (for example, the declaration in a header). + That part of the fix is only applied if the other file is also analyzed, is not ignored by + `ignore-tidy`, and the change is within the file's changed lines (per + [`lines-changed-only`][lines-changed-only]). + Otherwise, the review summary (and the thread comment) lists the parts that were not applied, + so they can be applied by hand. From 470100b508de1d415d0afefe51770dc8f39f6cb0 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Mon, 19 Oct 2026 07:59:37 +0000 Subject: [PATCH 2/4] fix: do not approve or say LGTM when some fix parts were not applied A review with skipped cross-file fix parts asks the user to apply them, so it should not approve the PR or say there are no concerns. --- cpp-linter/src/clang_tools/mod.rs | 4 +++- cpp-linter/src/rest_client.rs | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index d2ae8feb..268a5f38 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -373,8 +373,9 @@ impl ReviewComments { body.push_str(self.full_patch.as_str()); body.push_str(patch_suffix); } - } else if total_review_comments == 0 { + } else if total_review_comments == 0 && self.skipped_fixes.is_empty() { // Only congratulate if there was no reused comments + // and no fix parts left for the user to apply log::info!("No concerns to report: LGTM"); body.push_str("\nNo concerns to report. Great job! :tada:\n"); } @@ -499,6 +500,7 @@ mod tests { assert!( review_summary.contains("Please apply these parts yourself:\n\n- first\n- second\n") ); + assert!(!review_summary.contains("No concerns to report")); } #[test] diff --git a/cpp-linter/src/rest_client.rs b/cpp-linter/src/rest_client.rs index 8608d15f..c3399ba0 100644 --- a/cpp-linter/src/rest_client.rs +++ b/cpp-linter/src/rest_client.rs @@ -236,7 +236,8 @@ impl RestClient { let total_review_comments = options.comments.len() as u32; self.client.cull_pr_reviews(&mut options).await?; - let has_changes = review_comments.tool_total > 0; + let has_changes = + review_comments.tool_total > 0 || !review_comments.skipped_fixes.is_empty(); let is_lgtm = options.comments.is_empty() && !has_changes; options.action = if feedback_inputs.passive_reviews { ReviewAction::Comment From 73c6f1216a3ebbb0f063ce89508fb080872b4642 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Mon, 19 Oct 2026 18:11:34 +0000 Subject: [PATCH 3/4] fix: keep the skipped fixes list within the review summary's max length Only the list items that fit within GitHub's 65535 character limit (with room for the outreach text) are added; the number left out is noted. --- cpp-linter/src/clang_tools/mod.rs | 73 +++++++++++++++++++++++++++---- 1 file changed, 65 insertions(+), 8 deletions(-) diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index 268a5f38..f1e476a1 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -338,14 +338,7 @@ impl ReviewComments { format!("\n{dupes} suggestions were duplicates of previous reviews.\n").as_str(), ); } - if !self.skipped_fixes.is_empty() { - body.push_str( - "\nSome clang-tidy fixes also change files that cpp-linter did not change. \ - Please apply these parts yourself:\n\n", - ); - body.push_str(&self.skipped_fixes.join("\n")); - body.push('\n'); - } + self.summarize_skipped_fixes(&mut body); // The `full_patch` includes all suggestions that didn't fit in the diff. // It can also contain suggestions that were duplicates of previous reviews. if !self.full_patch.is_empty() { @@ -383,6 +376,42 @@ impl ReviewComments { body } + /// Append the list of [`Self::skipped_fixes`] to the review summary `body`. + /// + /// Only the list items that fit within the maximum length of a GitHub comment + /// (leaving room for [`USER_OUTREACH`]) are added. + /// The number of items left out is mentioned instead. + fn summarize_skipped_fixes(&self, body: &mut String) { + if self.skipped_fixes.is_empty() { + return; + } + const HEADING: &str = "\nSome clang-tidy fixes also change files that cpp-linter did not change. \ + Please apply these parts yourself:\n\n"; + // Room kept for the line about omitted items. + const OMITTED_RESERVE: usize = 100; + let max_len = (u16::MAX as usize).saturating_sub(USER_OUTREACH.len() + OMITTED_RESERVE); + if body.len() + HEADING.len() > max_len { + log::warn!("The list of skipped fixes is too large to include in the review summary."); + return; + } + body.push_str(HEADING); + let mut omitted = 0; + for item in &self.skipped_fixes { + if omitted == 0 && body.len() + item.len() < max_len { + body.push_str(item); + body.push('\n'); + } else { + omitted += 1; + } + } + if omitted > 0 { + log::warn!("{omitted} skipped fixes did not fit in the review summary."); + body.push_str(&format!( + "\n{omitted} more were not listed because this summary is too long.\n" + )); + } + } + /// Check if a given comment's [`Suggestion`] is already contained within the existing [`Self::comments`]. pub fn is_comment_in_suggestions(&mut self, comment: &Suggestion) -> bool { for s in &mut self.comments { @@ -503,6 +532,34 @@ mod tests { assert!(!review_summary.contains("No concerns to report")); } + #[test] + fn summarize_skipped_fixes_truncated() { + let clang_versions = ClangVersions { + format_version: None, + tidy_version: Some(PSEUDO_VERSION), + }; + let item = format!("- {}", "a".repeat(1000)); + let review_summary = ReviewComments { + skipped_fixes: vec![item; 100], + ..Default::default() + } + .summarize(&clang_versions, &[], 0, true); + assert!(review_summary.len() <= u16::MAX as usize); + assert!(review_summary.contains("more were not listed because this summary is too long.")); + assert!(review_summary.ends_with(USER_OUTREACH)); + } + + #[test] + fn summarize_skipped_fixes_no_room() { + let mut body = "a".repeat(u16::MAX as usize); + ReviewComments { + skipped_fixes: vec!["- first".to_string()], + ..Default::default() + } + .summarize_skipped_fixes(&mut body); + assert_eq!(body.len(), u16::MAX as usize); + } + #[test] fn summary_len_truncated() { let comments = vec![ReviewComment { From 1fce7503221928f4f43d025de0cf5a39a3ab1360 Mon Sep 17 00:00:00 2001 From: Xianpeng Shen <3353385+shenxianpeng@users.noreply.github.com> Date: Mon, 19 Oct 2026 18:34:29 +0000 Subject: [PATCH 4/4] fix: keep room for the oversized patch notice after the skipped fixes list --- cpp-linter/src/clang_tools/mod.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cpp-linter/src/clang_tools/mod.rs b/cpp-linter/src/clang_tools/mod.rs index f1e476a1..c092e3e3 100644 --- a/cpp-linter/src/clang_tools/mod.rs +++ b/cpp-linter/src/clang_tools/mod.rs @@ -278,6 +278,10 @@ pub struct ReviewComments { pub skipped_fixes: Vec, } +/// The notice added to a review summary instead of a patch that does not fit. +const PATCH_TOO_LARGE: &str = + "\nThe full patch of fixes is too large to include in this summary.\n"; + impl ReviewComments { /// Get a markdown-formatted string that summarizes the given [`ReviewComment`]s. /// @@ -358,9 +362,7 @@ impl ReviewComments { log::warn!( "The full patch of fixes is too large to include in the review summary." ); - body.push_str( - "\nThe full patch of fixes is too large to include in this summary.\n", - ); + body.push_str(PATCH_TOO_LARGE); } else { body.push_str(&patch_prefix); body.push_str(self.full_patch.as_str()); @@ -389,7 +391,9 @@ impl ReviewComments { Please apply these parts yourself:\n\n"; // Room kept for the line about omitted items. const OMITTED_RESERVE: usize = 100; - let max_len = (u16::MAX as usize).saturating_sub(USER_OUTREACH.len() + OMITTED_RESERVE); + // Also keep room for the notice that replaces a patch that does not fit. + let max_len = (u16::MAX as usize) + .saturating_sub(USER_OUTREACH.len() + OMITTED_RESERVE + PATCH_TOO_LARGE.len()); if body.len() + HEADING.len() > max_len { log::warn!("The list of skipped fixes is too large to include in the review summary."); return; @@ -541,10 +545,12 @@ mod tests { let item = format!("- {}", "a".repeat(1000)); let review_summary = ReviewComments { skipped_fixes: vec![item; 100], + full_patch: "a".repeat(1000), ..Default::default() } .summarize(&clang_versions, &[], 0, true); assert!(review_summary.len() <= u16::MAX as usize); + assert!(review_summary.contains(PATCH_TOO_LARGE)); assert!(review_summary.contains("more were not listed because this summary is too long.")); assert!(review_summary.ends_with(USER_OUTREACH)); }