Skip to content

feat: use diff instead of clang-format XML output #347

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

Merged
2bndy5 merged 2 commits into from
Jun 10, 2026
Merged

feat: use diff instead of clang-format XML output #347

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
1ead7f8
feat: use diff instead of clang-format XML output
2bndy5 Jun 10, 2026
646316c
use inclusive range and consider pure insertions
2bndy5 Jun 10, 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
11 changes: 0 additions & 11 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion cpp-linter/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ fast-glob = "1.0.1"
futures = "0.3.32"
gix-imara-diff = { version = "0.2.2", features = ["unified_diff"] }
log = { workspace = true }
quick-xml = { version = "0.40.1", features = ["serialize"] }
regex = { workspace = true }
reqwest = { workspace = true }
semver = { workspace = true }
Expand Down
209 changes: 67 additions & 142 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 @@ -3,28 +3,25 @@

use std::{
fs,
ops::RangeInclusive,
path::PathBuf,
process::Command,
sync::{Arc, Mutex, MutexGuard},
};

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

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

#[derive(Debug, Clone, Deserialize, PartialEq, Eq, Default)]
#[derive(Debug, Clone, PartialEq, Eq, Default)]
pub struct FormatAdvice {
/// A list of [`Replacement`]s that clang-tidy wants to make.
#[serde(rename(deserialize = "replacement"), default)]
pub replacements: Vec<Replacement>,
pub replacements: Vec<RangeInclusive<u32>>,

pub patched: Option<Vec<u8>>,
pub patched: PathBuf,
}

impl MakeSuggestions for FormatAdvice {
Expand All @@ -37,21 +34,6 @@ impl MakeSuggestions for FormatAdvice {
}
}

/// A single replacement that clang-format wants to make.
#[derive(Debug, PartialEq, Eq, Default, Clone, Copy, Deserialize)]
pub struct Replacement {
/// The byte offset where the replacement will start.
#[serde(rename = "@offset")]
pub offset: u32,

/// The line number described by the [`Replacement::offset`].
///
/// This value is not provided by the XML output, but we calculate it after
/// deserialization.
#[serde(default)]
pub line: u32,
}

/// Get a string that summarizes the given `--style`
pub fn summarize_style(style: &str) -> String {
let mut char_iter = style.chars();
Expand Down Expand Up @@ -97,49 +79,40 @@ pub fn run_clang_format(
for range in &ranges {
cmd.arg(format!("--lines={}:{}", range.start(), range.end()));
}
let cache_path = clang_params.project_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 file_name = file.name.to_string_lossy().to_string();
cmd.arg(file.name.to_path_buf().as_os_str());
let patched = if !clang_params.format_review {
None
} else {
logs.push((
Level::Info,
format!(
"Getting format fixes with \"{} {}\"",
cmd.get_program().to_string_lossy(),
cmd.get_args()
.map(|a| a.to_string_lossy())
.collect::<Vec<_>>()
.join(" ")
),
));
Some(
cmd.output()
.map_err(|e| ClangCaptureError::FailedToRunCommand {
task: format!("get fixes from clang-format {file_name}"),
source: e,
})?
.stdout,
)
};
cmd.arg("--output-replacements-xml");
logs.push((
log::Level::Info,
Level::Info,
format!(
"Running \"{} {}\"",
"Getting format fixes with \"{} {}\"",
cmd.get_program().to_string_lossy(),
cmd.get_args()
.map(|x| x.to_string_lossy())
.map(|a| a.to_string_lossy())
.collect::<Vec<_>>()
.join(" ")
),
));
let output = cmd
.output()
.map_err(|e| ClangCaptureError::FailedToRunCommand {
task: format!("Failed to get replacements from clang-format: {file_name}"),
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((
log::Level::Debug,
Expand All @@ -149,46 +122,47 @@ pub fn run_clang_format(
),
));
}
let mut format_advice = if !output.stdout.is_empty() {
let xml =
String::from_utf8(output.stdout).map_err(|e| ClangCaptureError::NonUtf8Output {
task: format!("XML output from clang-format (for {file_name})"),
source: e,
})?;
quick_xml::de::from_str::<FormatAdvice>(&xml).map_err(|e| {
ClangCaptureError::XmlParsingFailed {
file_name: file_name.clone(),
source: e,
}
})?
} else {
FormatAdvice::default()
};
format_advice.patched = patched;
if !format_advice.replacements.is_empty() {
let original_contents =
fs::read(&file.name).map_err(|e| ClangCaptureError::ReadFileFailed {
file_name: file_name.clone(),
source: e,
})?;
// get line and column numbers from format_advice.offset
let mut filtered_replacements = Vec::new();
for replacement in &mut format_advice.replacements {
let line_number = get_line_count_from_offset(&original_contents, replacement.offset);
replacement.line = line_number;
for range in &ranges {
if range.contains(&line_number) {
filtered_replacements.push(*replacement);
break;
}
}
if ranges.is_empty() {
// lines_changed_only is disabled
filtered_replacements.push(*replacement);
}

// use a diff between patched and original contents to get format results
let original_contents =
fs::read_to_string(&file.name).map_err(|e| ClangCaptureError::ReadFileFailed {
file_name: file_name.clone(),
source: e,
})?;
let patched_contents = String::from_utf8(output.stdout.to_vec()).map_err(|e| {
ClangCaptureError::NonUtf8Output {
task: "clang-format".to_string(),
source: e,
}
format_advice.replacements = filtered_replacements;
}
})?;
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 format_advice = FormatAdvice {
replacements: diff
.hunks()
.filter_map(|hunk| {
let replacement = if hunk.is_pure_insertion() {
RangeInclusive::new(hunk.after.start, hunk.after.end.saturating_sub(1))
} else {
RangeInclusive::new(hunk.before.start, hunk.before.end.saturating_sub(1))
};
if ranges.is_empty() {
Some(replacement)
} else {
// only include replacements that fall within the specified line ranges
if ranges.iter().any(|range| {
range.contains(replacement.start()) && range.contains(replacement.end())
}) {
Some(replacement)
} else {
None
}
}
})
.collect(),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
patched: cache_format_fixes,
};
file.format_advice = Some(format_advice);
Ok(logs)
}
Expand All @@ -197,56 +171,7 @@ pub fn run_clang_format(
mod tests {
#![allow(clippy::unwrap_used)]

use super::{FormatAdvice, Replacement, summarize_style};

#[test]
fn parse_blank_xml() {
let xml = String::new();
let result = quick_xml::de::from_str::<FormatAdvice>(&xml);
assert!(result.is_err());
}

#[test]
fn parse_xml_no_replacements() {
let xml_raw = r#"<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
</replacements>"#
.as_bytes()
.to_vec();
let expected = FormatAdvice::default();
let xml = String::from_utf8(xml_raw).unwrap();
let document = quick_xml::de::from_str::<FormatAdvice>(&xml).unwrap();
assert_eq!(expected, document);
}

#[test]
fn parse_xml() {
let xml_raw = r#"<?xml version='1.0'?>
<replacements xml:space='preserve' incomplete_format='false'>
<replacement offset='113' length='5'>&#10; </replacement>
<replacement offset='147' length='0'> </replacement>
<replacement offset='161' length='0'></replacement>
<replacement offset='165' length='19'>&#10;&#10;</replacement>
</replacements>"#
.as_bytes()
.to_vec();

let expected = FormatAdvice {
replacements: [113, 147, 161, 165]
.iter()
.map(|offset| Replacement {
offset: *offset,
..Default::default()
})
.collect(),
patched: None,
};

let xml = String::from_utf8(xml_raw).unwrap();

let document = quick_xml::de::from_str::<FormatAdvice>(&xml).unwrap();
assert_eq!(expected, document);
}
use super::summarize_style;

fn formalize_style(style: &str, expected: &str) {
assert_eq!(summarize_style(style), expected);
Expand Down
1 change: 1 addition & 0 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 @@ -496,6 +496,7 @@ mod test {
format_review: false,
clang_tidy_command: Some(exe_path),
clang_format_command: None,
project_cache_dir: PathBuf::from(".").join(".cpp-linter-cache"),
};
let mut file_lock = arc_file.lock().unwrap();
let logs = run_clang_tidy(&mut file_lock, &clang_params)
Expand Down
3 changes: 3 additions & 0 deletions cpp-linter/src/cli/structs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ pub struct ClangParams {
pub format_filter: Option<FileFilter>,
pub tidy_review: bool,
pub format_review: bool,
pub project_cache_dir: PathBuf,
}

#[cfg(feature = "bin")]
Expand Down Expand Up @@ -215,6 +216,8 @@ impl From<&Cli> for ClangParams {
format_filter,
tidy_review: args.feedback_options.tidy_review,
format_review: args.feedback_options.format_review,
project_cache_dir: PathBuf::from(&args.source_options.repo_root)
.join(".cpp-linter-cache"),
}
}
}
Expand Down
46 changes: 4 additions & 42 deletions cpp-linter/src/common_fs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,8 @@ impl FileObj {
) -> Result<(), FileObjError> {
let original_content = fs::read_to_string(&self.name).map_err(FileObjError::ReadFile)?;
let file_name = self.name.to_str().unwrap_or_default().replace("\\", "/");
if let Some(advice) = &self.format_advice
&& let Some(patched) = &advice.patched
{
let patched = String::from_utf8(patched.to_vec())
.map_err(|e| FileObjError::FromUtf8Error(file_name.clone(), e))?;
if let Some(advice) = &self.format_advice {
let patched = fs::read_to_string(&advice.patched).map_err(FileObjError::ReadFile)?;
let (diff, input) = make_patch(patched.as_str(), &original_content);
advice.get_suggestions(review_comments, self, &diff, &input, summary_only);
}
Expand Down Expand Up @@ -219,48 +216,13 @@ impl FileObj {
}
}

/// Gets the line number for a given `offset` (of bytes) from the given
/// buffer `contents`.
///
/// The `offset` given to this function is expected to originate from
/// diagnostic information provided by clang-format. Any `offset` out of
/// bounds is clamped to the given `contents` buffer's length.
pub fn get_line_count_from_offset(contents: &[u8], offset: u32) -> u32 {
let offset = (offset as usize).min(contents.len());
let lines = contents[0..offset].split(|byte| byte == &b'\n');
lines.count() as u32
}

#[cfg(test)]
mod test {
use std::{fs, path::PathBuf};
use std::path::PathBuf;

use super::{FileObj, get_line_count_from_offset};
use super::FileObj;
use crate::cli::LinesChangedOnly;

// *********************** tests for translating byte offset into line/column

#[test]
fn translate_byte_offset() {
let contents = fs::read(PathBuf::from("tests/demo/demo.cpp")).unwrap();
let lines = get_line_count_from_offset(&contents, 144);
assert_eq!(lines, 13);
}

#[test]
fn get_line_count_edge_cases() {
// Empty content
assert_eq!(get_line_count_from_offset(&[], 0), 1);

// No newlines
assert_eq!(get_line_count_from_offset(b"abc", 3), 1);

// Consecutive newlines
assert_eq!(get_line_count_from_offset(b"a\n\nb", 3), 3);

// Offset beyond content length
assert_eq!(get_line_count_from_offset(b"a\nb\n", 10), 3);
}
// *********************** tests for FileObj::get_ranges()

#[test]
Expand Down
Loading
Loading