fix: close behavior gaps with cpp-linter v1

cpp-linter/src/clang_tools/clang_tidy/mod.rs

@@ -191,7 +191,9 @@ fn parse_tidy_output(
                     for unit in db_json {
                         let unit_path =
                             PathBuf::from_iter([unit.directory.as_str(), unit.file.as_str()]);
-                        if unit_path == filename {
+                        // clang-tidy may report the path as it is written in the database
+                        // (relative to the unit's directory) or joined with the directory.
+                        if unit_path == filename || Path::new(&unit.file) == filename {
                             filename =
                                 normalize_path(&PathBuf::from_iter([&unit.directory, &unit.file]));
                             found_unit = true;

cpp-linter/src/clang_tools/clang_tidy/test.rs

@@ -299,6 +299,16 @@ fn parse_with_database_units() {
     assert_eq!(notes[0].filename, "demo.cpp");
     // path of a file not in the database is also made relative to the repo root
     assert_eq!(notes[1].filename, "unknown.cpp");
+
+    // clang-tidy can also report the path exactly as written in the database (eg. meson)
+    let notes = parse_tidy_output(
+        b"../demo.cpp:1:1: warning: some rationale [modernize-use-auto]\n",
+        &database_json,
+        &env::current_dir().unwrap(),
+    )
+    .unwrap();
+    assert_eq!(notes.len(), 1);
+    assert_eq!(notes[0].filename, "demo.cpp");
 }
 
 fn make_clang_params(

cpp-linter/src/cli/mod.rs

@@ -9,9 +9,7 @@ use std::str::FromStr;
 use clang_tools_manager::{RequestedVersion, logger::CLI_HELP_STYLE};
 #[cfg(feature = "bin")]
 use clap::{
-    ArgAction, Args, Parser, Subcommand, ValueEnum,
-    builder::{FalseyValueParser, NonEmptyStringValueParser},
-    value_parser,
+    ArgAction, Args, Parser, Subcommand, ValueEnum, builder::FalseyValueParser, value_parser,
 };
 
 mod structs;
@@ -22,8 +20,10 @@ pub use structs::{ClangParams, FeedbackInput, LinesChangedOnly, ThreadComments};
 #[derive(Debug, Clone, PartialEq, Eq, ValueEnum)]
 pub enum Verbosity {
     /// Enables the [`log::Level::Info`] log level and above.
+    #[value(alias = "20")]
     Info,
     /// Enables the [`log::Level::Debug`] log level and above.
+    #[value(alias = "10")]
     Debug,
 }
 
@@ -179,14 +179,17 @@ pub struct GeneralOptions {
 )]
 pub struct SourceOptions {
     /// A comma-separated list of file extensions to analyze.
+    ///
+    /// Surrounding whitespace and a leading `.` are ignored,
+    /// so `.cpp, .h` is the same as `cpp,h`.
     #[cfg_attr(
         feature = "bin",
         arg(
             short,
             long,
             value_delimiter = ',',
             default_value = "c,h,C,H,cpp,hpp,cc,hh,c++,h++,cxx,hxx",
-            value_parser = NonEmptyStringValueParser::new(),
+            value_parser = parse_extension,
             help_heading = "Source options"
         )
     )]
@@ -320,8 +323,9 @@ pub struct FormatOptions {
     ///   entirely.
     ///
     /// > [!NOTE]
-    /// > If this is not a blank string, then it is also passed to clang-tidy
-    /// > (if [`--tidy_checks`](#-c-tidy-checks) is not `-*`).
+    /// > If this is not a blank string, then fixes suggested by clang-tidy
+    /// > (if [`--tidy-checks`](#-c-tidy-checks) is not `-*`) are also
+    /// > formatted with this style.
     /// > This is done to ensure suggestions from both clang-tidy and
     /// > clang-format are consistent.
     #[cfg_attr(
@@ -574,6 +578,20 @@ pub struct FeedbackOptions {
     pub passive_reviews: bool,
 }
 
+/// Parses a single value of the `--extensions` option.
+///
+/// Surrounding whitespace and a leading `.` are removed.
+/// An empty value is an error.
+#[cfg(feature = "bin")]
+fn parse_extension(val: &str) -> Result<String, String> {
+    let ext = val.trim().trim_start_matches('.');
+    if ext.is_empty() {
+        Err(format!("invalid file extension: {val:?}"))
+    } else {
+        Ok(ext.to_string())
+    }
+}
+
 /// Converts the parsed value of the `--extra-arg` option into an optional vector of strings.
 ///
 /// This is for adapting to 2 scenarios where `--extra-arg` is either
@@ -603,12 +621,15 @@ pub fn convert_extra_arg_val(args: &[String]) -> Vec<String> {
         // specified once; split and return result
         v.trim_matches('\'')
             .trim_matches('"')
-            .split(' ')
+            .split_whitespace()
             .map(|i| i.to_string())
             .collect()
     } else {
-        // specified multiple times; just return a clone of the values
-        val.map(|i| i.to_string()).collect()
+        // specified multiple times; just strip any quotes and skip blank values
+        val.map(|i| i.trim_matches('\'').trim_matches('"').trim())
+            .filter(|i| !i.is_empty())
+            .map(|i| i.to_string())
+            .collect()
     }
 }
 
@@ -658,6 +679,38 @@ mod test {
         assert_eq!(extra_args, ["-std=c++17", "-Wall"])
     }
 
+    #[test]
+    fn extensions_are_normalized() {
+        let args = Cli::parse_from(vec!["cpp-linter", "-e", " .cpp, .h,hpp"]);
+        assert_eq!(args.source_options.extensions, ["cpp", "h", "hpp"]);
+        assert!(Cli::try_parse_from(vec!["cpp-linter", "-e", "c,.,h"]).is_err());
+    }
+
+    #[test]
+    fn numeric_verbosity() {
+        let args = Cli::parse_from(vec!["cpp-linter", "-v", "10"]);
+        assert!(args.general_options.verbosity.is_debug());
+        let args = Cli::parse_from(vec!["cpp-linter", "-v", "20"]);
+        assert!(!args.general_options.verbosity.is_debug());
+    }
+
+    #[test]
+    fn extra_arg_extra_spaces() {
+        let args = Cli::parse_from(vec!["cpp-linter", "--extra-arg=' -std=c++17   -Wall '"]);
+        let extra_args = convert_extra_arg_val(&args.tidy_options.extra_arg);
+        assert_eq!(extra_args, ["-std=c++17", "-Wall"]);
+
+        let args = Cli::parse_from(vec![
+            "cpp-linter",
+            "--extra-arg=\"-std=c++17\"",
+            "--extra-arg=",
+            "--extra-arg= ",
+            "--extra-arg='-Wall'",
+        ]);
+        let extra_args = convert_extra_arg_val(&args.tidy_options.extra_arg);
+        assert_eq!(extra_args, ["-std=c++17", "-Wall"]);
+    }
+
     #[test]
     fn extra_arg_2() {
         let args = Cli::parse_from(vec![

cpp-linter/src/common_fs.rs

@@ -250,7 +250,12 @@ impl FileObj {
             // Count of clang-tidy diagnostics that had no fixes applied
             let mut total = 0;
             for note in &advice.notes {
-                if note.fixed_lines.is_empty() && self.is_line_in_diff(&note.line) {
+                // Skip diagnostics about other files (eg. included headers);
+                // their line numbers do not refer to this file.
+                if note.filename == file_name
+                    && note.fixed_lines.is_empty()
+                    && self.is_line_in_diff(&note.line)
+                {
                     // notification had no suggestion applied in `patched`
                     total += 1;
                     if summary_only {
@@ -531,6 +536,11 @@ mod test {
                 note(1, vec![]),
                 // not in any existing suggestion; creates a new suggestion
                 note(3, vec!["line3".to_string()]),
+                // a diagnostic about an included header is not about this file's line 4
+                TidyNotification {
+                    filename: "test_file.h".to_string(),
+                    ..note(4, vec![])
+                },
             ],
         });
 

cpp-linter/src/rest_client.rs

@@ -206,6 +206,7 @@ impl RestClient {
             let summary_only = ["true", "on", "1"].contains(
                 &env::var("CPP_LINTER_PR_REVIEW_SUMMARY_ONLY")
                     .unwrap_or("false".to_string())
+                    .to_lowercase()
                     .as_str(),
             );
             let mut review_comments = ReviewComments::default();
@@ -236,9 +237,10 @@ impl RestClient {
             let total_review_comments = options.comments.len() as u32;
             self.client.cull_pr_reviews(&mut options).await?;
             let has_changes = review_comments.tool_total > 0;
+            let is_lgtm = options.comments.is_empty() && !has_changes;
             options.action = if feedback_inputs.passive_reviews {
                 ReviewAction::Comment
-            } else if options.comments.is_empty() && !has_changes {
+            } else if is_lgtm {
                 ReviewAction::Approve
             } else {
                 ReviewAction::RequestChanges
@@ -249,7 +251,15 @@ impl RestClient {
                 total_review_comments,
                 summary_only,
             );
-            self.client.post_pr_review(&options).await?;
+            if is_lgtm && feedback_inputs.no_lgtm {
+                log::debug!("Not posting an approved review because `no-lgtm` is true");
+            } else if options.comments.is_empty() && total_review_comments > 0 {
+                log::info!(
+                    "All review comments were already posted in previous reviews; not posting a new review."
+                );
+            } else {
+                self.client.post_pr_review(&options).await?;
+            }
         } else {
             for file in files {
                 let file = file
@@ -348,18 +358,22 @@ impl RestClient {
                 for note in &tidy_advice.notes {
                     let path = file.name.to_string_lossy().replace('\\', "/");
                     if note.filename == path {
-                        let title = format!("{}:{}:{}", note.filename, note.line, note.cols);
+                        let title = format!(
+                            "{}:{}:{} [{}]",
+                            note.filename, note.line, note.cols, note.diagnostic
+                        );
                         let annotation = FileAnnotation {
                             severity: match note.severity.as_str() {
                                 "warning" => AnnotationLevel::Warning,
                                 "error" => AnnotationLevel::Error,
                                 _ => AnnotationLevel::Notice, // default to notice for all else
                             },
                             path,
-                            start_line: None,
+                            // `line` is required to anchor the annotation to a line in the file
+                            start_line: Some(note.line as usize),
                             end_line: Some(note.line as usize),
-                            start_column: None,
-                            end_column: Some(note.cols as usize),
+                            start_column: Some(note.cols as usize),
+                            end_column: None,
                             title: Some(title),
                             message: note.rationale.clone(),
                         };
@@ -524,7 +538,7 @@ fn make_tidy_comment(
     remaining_length: &mut u64,
 ) -> Result<(), ClientError> {
     let opener = format!(
-        "\n<details><summary>clang-tidy (v{version_used}) reports: {tidy_checks_failed}<strong> concern(s)</strong></summary>\n\n"
+        "\n<details><summary>clang-tidy (v{version_used}) reports: <strong>{tidy_checks_failed} concern(s)</strong></summary>\n\n"
     );
     let mut show_fix_explanation = false;
     let mut tidy_comment = String::new();
@@ -800,6 +814,26 @@ mod test {
         );
     }
 
+    #[test]
+    fn tidy_annotation_points_to_line() {
+        let annotations =
+            RestClient::make_annotations(&make_dummy_files("warning"), "llvm").unwrap();
+        let tidy = annotations
+            .iter()
+            .find(|a| matches!(a.severity, AnnotationLevel::Warning))
+            .unwrap();
+        assert_eq!(tidy.start_line, Some(1));
+        assert_eq!(tidy.start_column, Some(1));
+        assert_eq!(
+            tidy.title.as_deref(),
+            Some("tests/demo/demo.cpp:1:1 [clang-diagnostic-error]")
+        );
+        assert!(
+            tidy.fmt_github()
+                .starts_with("::warning file=tests/demo/demo.cpp,line=1,col=1,")
+        );
+    }
+
     #[test]
     fn comment_without_tidy_concerns() {
         let files = make_dummy_files("warning");

cpp-linter/src/run.rs

@@ -67,6 +67,9 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
         },
     );
     let is_pr = rest_api_client.is_pr();
+    if !is_pr && cli.feedback_options.pr_review && rest_api_client.client_kind() == "local" {
+        log::warn!("Pull request reviews are only posted when running in a supported CI platform.");
+    }
 
     let mut file_filter = FileFilter::new(
         &cli.source_options
@@ -108,7 +111,7 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
     }
 
     rest_api_client.start_log_group("Get list of specified source files");
-    let files = if !matches!(cli.source_options.lines_changed_only, LinesChangedOnly::Off)
+    let mut files = if !matches!(cli.source_options.lines_changed_only, LinesChangedOnly::Off)
         || cli.source_options.files_changed_only
     {
         // parse_diff(github_rest_api_payload)
@@ -151,6 +154,9 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
         }
         all_files
     };
+    // The file lists from git and the directory walk are unordered.
+    // Sort them, so the order of feedback (comments, annotations, etc) is deterministic.
+    files.sort_by(|a, b| a.name.cmp(&b.name));
     let mut arc_files = vec![];
     log::info!("Giving attention to the following files:");
     for file in files {
@@ -195,7 +201,7 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
         .post_feedback(&arc_files, user_inputs, clang_versions)
         .await?;
     rest_api_client.end_log_group("Posting feedback");
-    if env::var("PRE_COMMIT").is_ok_and(|v| v == "1") && checks_failed > 1 {
+    if env::var("PRE_COMMIT").is_ok_and(|v| v == "1") && checks_failed > 0 {
         return Err(anyhow!("Some checks did not pass"));
     }
     Ok(())
@@ -238,6 +244,28 @@ pub(crate) mod test {
         drop(tmp_gh_out);
     }
 
+    #[tokio::test]
+    async fn pr_review_outside_ci() {
+        let tmp_gh_out = setup_tmp_gh_out_path();
+        let tmp_workspace = setup_tmp_workspace();
+        unsafe {
+            env::set_var("GITHUB_ACTIONS", "false");
+        }
+        // a PR review is requested, but only a warning is logged outside CI
+        run_main(vec![
+            "cpp-linter".to_string(),
+            "-l".to_string(),
+            "false".to_string(),
+            "--pr-review".to_string(),
+            "--repo-root".to_string(),
+            tmp_workspace.path().to_str().unwrap().to_string(),
+            "demo/demo.cpp".to_string(),
+        ])
+        .await
+        .unwrap();
+        drop(tmp_gh_out);
+    }
+
     #[tokio::test]
     async fn no_mod_sys() {
         let tmp_gh_out = setup_tmp_gh_out_path();