improve error handling when parsing XML

Author: 2bndy5

cpp-linter/src/clang_tools/clang_format.rs

@@ -94,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!(
@@ -112,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,
@@ -142,16 +143,19 @@ pub fn run_clang_format(
             ),
         ));
     }
-    if output.stdout.is_empty() {
-        return Ok(logs);
-    }
-    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,
-    });
+    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(|| {
@@ -185,6 +189,13 @@ pub fn run_clang_format(
 mod tests {
     use super::{summarize_style, FormatAdvice, Replacement};
 
+    #[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() {
         let xml_raw = r#"<?xml version='1.0'?>

cpp-linter/src/clang_tools/clang_tidy.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()]);
     }

cpp-linter/src/clang_tools/mod.rs

@@ -258,20 +258,18 @@ pub struct ReviewComments {
 impl ReviewComments {
     pub fn summarize(&self, clang_versions: &ClangVersions) -> String {
         let mut body = format!("{COMMENT_MARKER}## Cpp-linter Review\n");
-        for t in 0u8..=1 {
+        for t in 0_usize..=1 {
             let mut total = 0;
             let (tool_name, tool_version) = if t == 0 {
                 ("clang-format", clang_versions.format_version.as_ref())
             } else {
                 ("clang-tidy", clang_versions.tidy_version.as_ref())
             };
-
-            let tool_total = if let Some(total) = self.tool_total[t as usize] {
-                total
-            } else {
-                // review was not requested from this tool or the tool was not used at all
+            if tool_version.is_none() {
+                // this tool was not used at all
                 continue;
-            };
+            }
+            let tool_total = self.tool_total[t].unwrap_or_default();
 
             // If the tool's version is unknown, then we don't need to output this line.
             // NOTE: If the tool was invoked at all, then the tool's version shall be known.
@@ -295,11 +293,11 @@ impl ReviewComments {
                     .as_str(),
                 );
             }
-            if !self.full_patch[t as usize].is_empty() {
+            if !self.full_patch[t].is_empty() {
                 body.push_str(
                     format!(
                         "\n<details><summary>Click here for the full {tool_name} patch</summary>\n\n```diff\n{}```\n\n</details>\n",
-                        self.full_patch[t as usize]
+                        self.full_patch[t]
                     ).as_str()
                 );
             } else {
@@ -370,8 +368,7 @@ pub trait MakeSuggestions {
         patch: &mut Patch,
         summary_only: bool,
     ) -> Result<()> {
-        let tool_name = self.get_tool_name();
-        let is_tidy_tool = tool_name == "clang-tidy";
+        let is_tidy_tool = (&self.get_tool_name() == "clang-tidy") as usize;
         let hunks_total = patch.num_hunks();
         let mut hunks_in_patch = 0u32;
         let file_name = file_obj
@@ -384,13 +381,13 @@ pub trait MakeSuggestions {
             .to_buf()
             .with_context(|| "Failed to convert patch to byte array")?
             .to_vec();
-        review_comments.full_patch[is_tidy_tool as usize].push_str(
+        review_comments.full_patch[is_tidy_tool].push_str(
             String::from_utf8(patch_buf.to_owned())
                 .with_context(|| format!("Failed to convert patch to string: {file_name}"))?
                 .as_str(),
         );
-        review_comments.tool_total[is_tidy_tool as usize].get_or_insert(0);
         if summary_only {
+            review_comments.tool_total[is_tidy_tool].get_or_insert(0);
             return Ok(());
         }
         for hunk_id in 0..hunks_total {
@@ -447,9 +444,8 @@ pub trait MakeSuggestions {
                 review_comments.comments.push(comment);
             }
         }
-        review_comments.tool_total[is_tidy_tool as usize] = Some(
-            review_comments.tool_total[is_tidy_tool as usize].unwrap_or_default() + hunks_in_patch,
-        );
+        review_comments.tool_total[is_tidy_tool] =
+            Some(review_comments.tool_total[is_tidy_tool].unwrap_or_default() + hunks_in_patch);
         Ok(())
     }
 }

cpp-linter/src/common_fs/file_filter.rs

@@ -111,7 +111,7 @@ impl FileFilter {
                 || (pat.is_dir() && file_name.starts_with(pat))
             {
                 log::debug!(
-                    "file {file_name:?} is in {}ignored with domain {pattern:?}.",
+                    "file {file_name:?} is {}ignored with domain {pattern:?}.",
                     if is_ignored { "" } else { "not " }
                 );
                 return true;