Skip to content

feat: merge review suggestions from both clang tools #358

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.

Sign up for GitHub

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

Draft
2bndy5 wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

feat: merge review suggestions from both clang tools #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1751ae1
feat: merge review suggestions from both clang tools
2bndy5 Jun 14, 2026
646fc1c
adjust test expectation about changed lines
2bndy5 Jun 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 84 additions & 35 deletions cpp-linter/src/clang_tools/clang_format.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,36 +4,22 @@
use std::{
fs,
ops::RangeInclusive,
path::PathBuf,
process::Command,
sync::{Arc, Mutex, MutexGuard},
};

use gix_imara_diff::{Diff, InternedInput};
use log::Level;

// project-specific crates/modules
use super::{CACHE_DIR, MakeSuggestions};
use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError};
use crate::{
clang_tools::make_patch, cli::ClangParams, common_fs::FileObj, error::ClangCaptureError,
};

/// A struct to hold clang-format advice for a single file.
#[derive(Debug, Clone, PartialEq, Eq, Default)]
pub struct FormatAdvice {
/// A list of line ranges that clang-format wants to replace.
pub replacements: Vec<RangeInclusive<u32>>,

/// A path to a cached file containing the full contents of the file after applying clang-format fixes.
pub patched: PathBuf,
}

impl MakeSuggestions for FormatAdvice {
fn get_suggestion_help(&self, _start_line: u32, _end_line: u32) -> String {
String::from("### clang-format suggestions\n")
}

fn get_tool_name(&self) -> String {
"clang-format".to_string()
}
}

/// Get a string that summarizes the given `--style`
Expand Down Expand Up @@ -82,14 +68,7 @@ pub fn run_clang_format(
for range in &ranges {
cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
}
let cache_path = clang_params.repo_root.join(CACHE_DIR).join("patches");
let cache_format_fixes = cache_path.join(file.name.with_added_extension("format"));
fs::create_dir_all(
cache_format_fixes
.parent()
.ok_or(ClangCaptureError::UnknownCacheParentPath)?,
)
.map_err(ClangCaptureError::MkDirFailed)?;
let cache_path = clang_params.get_cache_path();
let file_name = file.name.to_string_lossy().to_string();
cmd.arg(file.name.to_path_buf().as_os_str());
logs.push((
Expand All @@ -109,12 +88,6 @@ pub fn run_clang_format(
task: format!("get fixes from clang-format {file_name}"),
source: e,
})?;
fs::write(&cache_format_fixes, &output.stdout).map_err(|e| {
ClangCaptureError::WriteFileFailed {
file_name: cache_format_fixes.to_string_lossy().to_string(),
source: e,
}
})?;

if !output.stderr.is_empty() || !output.status.success() {
logs.push((
Expand All @@ -140,9 +113,7 @@ pub fn run_clang_format(
source: e,
}
})?;
let input = InternedInput::new(original_contents.as_str(), patched_contents.as_str());
let mut diff = Diff::compute(gix_imara_diff::Algorithm::Histogram, &input);
diff.postprocess_lines(&input);
let (diff, _) = make_patch(&patched_contents, &original_contents);
let format_advice = FormatAdvice {
replacements: diff
.hunks()
Expand All @@ -166,8 +137,86 @@ pub fn run_clang_format(
}
})
.collect(),
patched: cache_format_fixes,
};

// if a clang-tidy patched file exists in cache,
// get the diff between it and the original file,
// then format both clang-tidy fixes and any other changes by clang-format fixes.
if let Some(patched_path) = &file.patched_path
&& patched_path.exists()
{
let tidy_patch_contents =
fs::read_to_string(patched_path).map_err(|e| ClangCaptureError::ReadFileFailed {
file_name: patched_path.to_string_lossy().to_string(),
source: e,
})?;
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"]);
// 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().after),
// 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.after.end
.map(|hunk| RangeInclusive::new(hunk.after.start, hunk.after.end.saturating_sub(1)))
.collect::<Vec<_>>();
for range in &ranges {
let mut contained = false;
for hunk in tidy_diff.hunks() {
if hunk.after.contains(range.start()) && hunk.after.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());
}
}
cmd.arg(&file_name);
let output = cmd
.output()
.map_err(|e| ClangCaptureError::FailedToRunCommand {
task: format!("apply clang-format to clang-tidy fixes ({file_name})"),
source: e,
})?;
if !output.stderr.is_empty() || !output.status.success() {
logs.push((
log::Level::Debug,
format!(
"clang-format raised the follow errors about clang-tidy fixes:\n{}",
String::from_utf8_lossy(&output.stderr)
),
));
}
} else {
// clang-tidy was not run on this file,
// so just use the clang-format fixes as the patched content.
let cache_format_fixes = cache_path.join(&file.name);
fs::create_dir_all(
cache_format_fixes
.parent()
.ok_or(ClangCaptureError::UnknownCacheParentPath)?,
)
.map_err(ClangCaptureError::MkDirFailed)?;
fs::write(&cache_format_fixes, &output.stdout).map_err(|e| {
ClangCaptureError::WriteFileFailed {
file_name: cache_format_fixes.to_string_lossy().to_string(),
source: e,
}
})?;
file.patched_path = Some(cache_format_fixes);
}

file.format_advice = Some(format_advice);
Ok(logs)
}
Expand Down
29 changes: 8 additions & 21 deletions cpp-linter/src/clang_tools/clang_tidy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@ use regex::Regex;
use serde::Deserialize;

// project-specific modules/crates
use super::MakeSuggestions;
use crate::{
clang_tools::CACHE_DIR, cli::ClangParams, common_fs::FileObj, error::ClangCaptureError,
};
use crate::{cli::ClangParams, common_fs::FileObj, error::ClangCaptureError};

/// Used to deserialize a json compilation database's translation unit.
///
Expand Down Expand Up @@ -104,13 +101,10 @@ impl TidyNotification {
pub struct TidyAdvice {
/// A list of notifications parsed from clang-tidy stdout.
pub notes: Vec<TidyNotification>,

/// A path to the cached contents of the file after applying clang-tidy fixes.
pub patched: PathBuf,
}

impl MakeSuggestions for TidyAdvice {
fn get_suggestion_help(&self, start_line: u32, end_line: u32) -> String {
impl TidyAdvice {
pub(crate) fn get_suggestion_help(&self, start_line: u32, end_line: u32) -> String {
let mut diagnostics = vec![];
for note in &self.notes {
for fixed_line in &note.fixed_lines {
Expand All @@ -133,10 +127,6 @@ impl MakeSuggestions for TidyAdvice {
diagnostics.join("")
)
}

fn get_tool_name(&self) -> String {
"clang-tidy".to_string()
}
}

/// A regex pattern to capture the clang-tidy note header.
Expand Down Expand Up @@ -336,10 +326,8 @@ pub fn run_clang_tidy(
.join(" ")
),
));
let cache_patch_path = clang_params
.repo_root
.join(CACHE_DIR)
.join(file.name.with_added_extension("tidy"));
let cache_path = clang_params.get_cache_path();
let cache_patch_path = cache_path.join(&file.name);
fs::create_dir_all(
cache_patch_path
.parent()
Expand Down Expand Up @@ -403,10 +391,8 @@ pub fn run_clang_tidy(
&clang_params.repo_root,
)?;

let tidy_advice = TidyAdvice {
notes,
patched: cache_patch_path.to_path_buf(),
};
let tidy_advice = TidyAdvice { notes };
file.patched_path = Some(cache_patch_path.to_path_buf());
file.tidy_advice = Some(tidy_advice);
Ok(logs)
}
Expand Down Expand Up @@ -539,6 +525,7 @@ mod test {
repo_root: tmp_workspace.path().to_path_buf(),
};
let mut file_lock = arc_file.lock().unwrap();
fs::create_dir_all(clang_params.get_cache_path()).unwrap();
let logs = run_clang_tidy(&mut file_lock, &clang_params)
.unwrap()
.into_iter()
Expand Down
Loading
Loading