refactor: switch to quick_xml library (#101)

* use `quick_xml`
* deserialize only what is needed from XML. The `cols`, `length`, and `value` fields were never used and are now ignored when parsing XML
* read file contents only once when translating byte offset to line
* improve error handling when parsing XML

Author: 2bndy5

Cargo.lock

@@ -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"

cpp-linter/Cargo.toml

@@ -2,29 +2,26 @@
 //! 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;
 use crate::{
     cli::ClangParams,
-    common_fs::{get_line_cols_from_offset, FileObj},
+    common_fs::{get_line_count_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, PartialEq, Eq)]
 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,43 +38,18 @@ impl MakeSuggestions for FormatAdvice {
 }
 
 /// A single replacement that clang-format wants to make.
-#[derive(Debug, Deserialize, PartialEq)]
+#[derive(Debug, PartialEq, Eq, Default, Clone, Copy, Deserialize)]
 pub struct Replacement {
     /// The byte offset where the replacement will start.
-    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>,
+    #[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: usize,
-
-    /// The column number on the line described by the [`Replacement::offset`].
-    ///
-    /// This value is not provided by the XML output, but we calculate it after
-    /// deserialization.
-    #[serde(default)]
-    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,
-        }
-    }
+    pub line: u32,
 }
 
 /// Get a string that summarizes the given `--style`
@@ -122,8 +94,9 @@ pub fn run_clang_format(
     }
     let file_name = file.name.to_string_lossy().to_string();
     cmd.arg(file.name.to_path_buf().as_os_str());
-    let mut patched = None;
-    if clang_params.format_review {
+    let patched = if !clang_params.format_review {
+        None
+    } else {
         logs.push((
             Level::Info,
             format!(
@@ -140,12 +113,12 @@ pub fn run_clang_format(
                     .join(" ")
             ),
         ));
-        patched = Some(
+        Some(
             cmd.output()
                 .with_context(|| format!("Failed to get fixes from clang-format: {file_name}"))?
                 .stdout,
-        );
-    }
+        )
+    };
     cmd.arg("--output-replacements-xml");
     logs.push((
         log::Level::Info,
@@ -170,41 +143,40 @@ 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 {
+    let mut format_advice = if !output.stdout.is_empty() {
+        let xml = String::from_utf8(output.stdout).with_context(|| {
+            format!("XML output from clang-format was not UTF-8 encoded: {file_name}")
+        })?;
+        quick_xml::de::from_str::<FormatAdvice>(&xml).with_context(|| {
+            format!("Failed to parse XML output from clang-format for {file_name}")
+        })?
+    } else {
+        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 read file's original content before translating byte offsets: {file_name}",
+            )
+        })?;
         // 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 = get_line_count_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());
+                if range.contains(&line_number) {
+                    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 +188,13 @@ pub fn run_clang_format(
 #[cfg(test)]
 mod tests {
     use super::{summarize_style, FormatAdvice, Replacement};
-    use serde::Deserialize;
+
+    #[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() {
@@ -226,51 +204,24 @@ 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,
-                },
-                Replacement {
-                    offset: 147,
-                    length: 0,
-                    value: Some(String::from(" ")),
-                    line: 0,
-                    cols: 0,
-                },
-                Replacement {
-                    offset: 161,
-                    length: 0,
-                    value: None,
-                    line: 0,
-                    cols: 0,
-                },
-                Replacement {
-                    offset: 165,
-                    length: 19,
-                    value: Some(String::from("\n\n")),
-                    line: 0,
-                    cols: 0,
-                },
-            ],
+            replacements: [113, 147, 161, 165]
+                .iter()
+                .map(|offset| Replacement {
+                    offset: *offset,
+                    ..Default::default()
+                })
+                .collect(),
             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();
+
+        let xml = String::from_utf8(xml_raw).unwrap();
+
+        let document = quick_xml::de::from_str::<FormatAdvice>(&xml).unwrap();
         assert_eq!(expected, document);
     }
 

cpp-linter/src/clang_tools/clang_format.rs

@@ -276,16 +276,17 @@ pub fn run_clang_tidy(
             cmd.args(["--line-filter", filter.as_str()]);
         }
     }
-    let mut original_content = None;
-    if clang_params.tidy_review {
+    let original_content = if !clang_params.tidy_review {
+        None
+    } else {
         cmd.arg("--fix-errors");
-        original_content = Some(fs::read_to_string(&file.name).with_context(|| {
+        Some(fs::read_to_string(&file.name).with_context(|| {
             format!(
                 "Failed to cache file's original content before applying clang-tidy changes: {}",
                 file_name.clone()
             )
-        })?);
-    }
+        })?)
+    };
     if !clang_params.style.is_empty() {
         cmd.args(["--format-style", clang_params.style.as_str()]);
     }