use quick_xml

2bndy5 · 2bndy5 · commit 9e11645e77a3 · 2025-01-20T22:05:44.000-08:00
- deserialize only what is needed from XML. `length` and `value` fields are not used and are now ignored when parsing XML
- read file contents only once when translating byte offset to line and columns
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/cpp-linter/Cargo.toml b/cpp-linter/Cargo.toml
@@ -25,11 +25,11 @@ lenient_semver = "0.4.2"
 log = { version = "0.4.25", features = ["std"] }
 openssl = { version = "0.10", features = ["vendored"], optional = true }
 openssl-probe = { version = "0.1", optional = true }
+quick-xml = {version = "0.37.2", features = ["serialize"]}
 regex = "1.11.1"
 reqwest = "0.12.12"
 semver = "1.0.25"
 serde = { version = "1.0.217", features = ["derive"] }
-serde-xml-rs = "0.6.0"
 serde_json = "1.0.137"
 tokio = { version = "1.43.0", features = ["macros", "rt-multi-thread"]}
 tokio-macros = "2.4.0"
diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs
@@ -2,15 +2,14 @@
 //! output.
 
 use std::{
+    fs,
     process::Command,
     sync::{Arc, Mutex, MutexGuard},
 };
 
 use anyhow::{Context, Result};
 use log::Level;
-// non-std crates
 use serde::Deserialize;
-use serde_xml_rs::de::Deserializer;
 
 // project-specific crates/modules
 use super::MakeSuggestions;
@@ -19,12 +18,10 @@ use crate::{
     common_fs::{get_line_cols_from_offset, FileObj},
 };
 
-/// A Structure used to deserialize clang-format's XML output.
-#[derive(Debug, Deserialize, PartialEq, Clone)]
-#[serde(rename = "replacements")]
+#[derive(Debug, Clone, Deserialize)]
 pub struct FormatAdvice {
     /// A list of [`Replacement`]s that clang-tidy wants to make.
-    #[serde(rename = "$value")]
+    #[serde(rename(deserialize = "replacement"))]
     pub replacements: Vec<Replacement>,
 
     pub patched: Option<Vec<u8>>,
@@ -41,18 +38,12 @@ impl MakeSuggestions for FormatAdvice {
 }
 
 /// A single replacement that clang-format wants to make.
-#[derive(Debug, Deserialize, PartialEq)]
+#[derive(Debug, PartialEq, Default, Clone, Copy, Deserialize)]
 pub struct Replacement {
     /// The byte offset where the replacement will start.
+    #[serde(rename = "@offset")]
     pub offset: usize,
 
-    /// The amount of bytes that will be removed.
-    pub length: usize,
-
-    /// The bytes (UTF-8 encoded) that will be added at the [`Replacement::offset`] position.
-    #[serde(rename = "$value")]
-    pub value: Option<String>,
-
     /// The line number described by the [`Replacement::offset`].
     ///
     /// This value is not provided by the XML output, but we calculate it after
@@ -68,18 +59,6 @@ pub struct Replacement {
     pub cols: usize,
 }
 
-impl Clone for Replacement {
-    fn clone(&self) -> Self {
-        Replacement {
-            offset: self.offset,
-            length: self.length,
-            value: self.value.clone(),
-            line: self.line,
-            cols: self.cols,
-        }
-    }
-}
-
 /// Get a string that summarizes the given `--style`
 pub fn summarize_style(style: &str) -> String {
     if ["google", "chromium", "microsoft", "mozilla", "webkit"].contains(&style) {
@@ -173,38 +152,37 @@ pub fn run_clang_format(
     if output.stdout.is_empty() {
         return Ok(logs);
     }
-    let xml = String::from_utf8(output.stdout)
-        .with_context(|| format!("stdout from clang-format was not UTF-8 encoded: {file_name}"))?
-        .lines()
-        .collect::<Vec<&str>>()
-        .join("");
-    let config = serde_xml_rs::ParserConfig::new()
-        .trim_whitespace(false)
-        .whitespace_to_characters(true)
-        .ignore_root_level_whitespace(true);
-    let event_reader = serde_xml_rs::EventReader::new_with_config(xml.as_bytes(), config);
-    let mut format_advice = FormatAdvice::deserialize(&mut Deserializer::new(event_reader))
-        .unwrap_or(FormatAdvice {
-            replacements: vec![],
-            patched: None,
-        });
+    let xml = String::from_utf8(output.stdout).with_context(|| {
+        format!("XML output from clang-format was not UTF-8 encoded: {file_name}")
+    })?;
+    let mut format_advice = quick_xml::de::from_str::<FormatAdvice>(&xml).unwrap_or(FormatAdvice {
+        replacements: vec![],
+        patched: None,
+    });
     format_advice.patched = patched;
     if !format_advice.replacements.is_empty() {
+        let original_contents = fs::read(&file.name).with_context(|| {
+            format!(
+                "Failed to cache file's original content before applying clang-tidy changes: {}",
+                file_name.clone()
+            )
+        })?;
         // 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, columns) = get_line_cols_from_offset(&file.name, replacement.offset);
+            let (line_number, columns) =
+                get_line_cols_from_offset(&original_contents, replacement.offset);
             replacement.line = line_number;
             replacement.cols = columns;
             for range in &ranges {
                 if range.contains(&line_number.try_into().unwrap_or(0)) {
-                    filtered_replacements.push(replacement.clone());
+                    filtered_replacements.push(*replacement);
                     break;
                 }
             }
             if ranges.is_empty() {
                 // lines_changed_only is disabled
-                filtered_replacements.push(replacement.clone());
+                filtered_replacements.push(*replacement);
             }
         }
         format_advice.replacements = filtered_replacements;
@@ -216,7 +194,6 @@ pub fn run_clang_format(
 #[cfg(test)]
 mod tests {
     use super::{summarize_style, FormatAdvice, Replacement};
-    use serde::Deserialize;
 
     #[test]
     fn parse_xml() {
@@ -226,52 +203,42 @@ mod tests {
 <replacement offset='147' length='0'> </replacement>
 <replacement offset='161' length='0'></replacement>
 <replacement offset='165' length='19'>&#10;&#10;</replacement>
-</replacements>"#;
-        //since whitespace is part of the elements' body, we need to remove the LFs first
-        let xml = xml_raw.lines().collect::<Vec<&str>>().join("");
+</replacements>"#
+            .as_bytes()
+            .to_vec();
 
         let expected = FormatAdvice {
             replacements: vec![
                 Replacement {
                     offset: 113,
-                    length: 5,
-                    value: Some(String::from("\n      ")),
-                    line: 0,
-                    cols: 0,
+                    ..Default::default()
                 },
                 Replacement {
                     offset: 147,
-                    length: 0,
-                    value: Some(String::from(" ")),
-                    line: 0,
-                    cols: 0,
+                    ..Default::default()
                 },
                 Replacement {
                     offset: 161,
-                    length: 0,
-                    value: None,
-                    line: 0,
-                    cols: 0,
+                    ..Default::default()
                 },
                 Replacement {
                     offset: 165,
-                    length: 19,
-                    value: Some(String::from("\n\n")),
-                    line: 0,
-                    cols: 0,
+                    ..Default::default()
                 },
             ],
             patched: None,
         };
-        let config = serde_xml_rs::ParserConfig::new()
-            .trim_whitespace(false)
-            .whitespace_to_characters(true)
-            .ignore_root_level_whitespace(true);
-        let event_reader = serde_xml_rs::EventReader::new_with_config(xml.as_bytes(), config);
-        let document =
-            FormatAdvice::deserialize(&mut serde_xml_rs::de::Deserializer::new(event_reader))
-                .unwrap();
-        assert_eq!(expected, document);
+
+        let xml = String::from_utf8(xml_raw).unwrap();
+
+        let document = quick_xml::de::from_str::<FormatAdvice>(&xml).unwrap();
+        assert_eq!(expected.replacements.len(), document.replacements.len());
+        for i in 0..expected.replacements.len() {
+            assert_eq!(
+                expected.replacements[i].offset,
+                document.replacements[i].offset
+            );
+        }
     }
 
     fn formalize_style(style: &str, expected: &str) {
diff --git a/cpp-linter/src/common_fs/mod.rs b/cpp-linter/src/common_fs/mod.rs
@@ -2,7 +2,6 @@
 
 use std::fmt::Debug;
 use std::fs;
-use std::io::Read;
 use std::path::{Component, Path};
 use std::{ops::RangeInclusive, path::PathBuf};
 
@@ -228,15 +227,11 @@ impl FileObj {
 /// boundary exists at the returned column number. However, the `offset` given to this
 /// function is expected to originate from diagnostic information provided by
 /// clang-format or clang-tidy.
-pub fn get_line_cols_from_offset(file_path: &PathBuf, offset: usize) -> (usize, usize) {
-    let mut file_buf = vec![0; offset];
-    fs::File::open(file_path)
-        .unwrap()
-        .read_exact(&mut file_buf)
-        .unwrap();
-    let lines = file_buf.split(|byte| byte == &b'\n');
+pub fn get_line_cols_from_offset(contents: &[u8], offset: usize) -> (usize, usize) {
+    let lines = contents[0..offset].split(|byte| byte == &b'\n');
     let line_count = lines.clone().count();
-    let column_count = lines.last().unwrap_or(&[]).len() + 1; // +1 because not a 0 based count
+    // here we `cols.len() + 1` because columns is not a 0-based count
+    let column_count = lines.last().map(|cols| cols.len() + 1).unwrap_or(1);
     (line_count, column_count)
 }
 
@@ -272,8 +267,8 @@ pub fn normalize_path(path: &Path) -> PathBuf {
 
 #[cfg(test)]
 mod test {
-    use std::env::current_dir;
     use std::path::PathBuf;
+    use std::{env::current_dir, fs};
 
     use super::{get_line_cols_from_offset, normalize_path, FileObj};
     use crate::cli::LinesChangedOnly;
@@ -317,7 +312,8 @@ mod test {
 
     #[test]
     fn translate_byte_offset() {
-        let (lines, cols) = get_line_cols_from_offset(&PathBuf::from("tests/demo/demo.cpp"), 144);
+        let contents = fs::read(PathBuf::from("tests/demo/demo.cpp")).unwrap();
+        let (lines, cols) = get_line_cols_from_offset(&contents, 144);
         println!("lines: {lines}, cols: {cols}");
         assert_eq!(lines, 13);
         assert_eq!(cols, 5);
diff --git a/cpp-linter/src/rest_api/github/mod.rs b/cpp-linter/src/rest_api/github/mod.rs
@@ -316,8 +316,6 @@ mod test {
                 });
                 let replacements = vec![Replacement {
                     offset: 0,
-                    length: 0,
-                    value: Some(String::new()),
                     line: 1,
                     cols: 1,
                 }];
