[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

What should happen with the following code:

  // NOLINTBEGIN(google*)
  
  // NOLINTEND(google-explicit-constructor)
  // NOLINTEND(google-runtime-operator)

and this code:

  // NOLINTBEGIN(google-explicit-constructor)
  // NOLINTBEGIN(google-runtime-operator)
  
  // NOLINTEND(google*)

At the moment, Clang-Tidy does not allow mixing "check-specific" `NOLINTBEGIN/END`s with "global" ones. See discussion in D108560#3020444 <https://reviews.llvm.org/D108560#3020444>. So the following is not a supported construct:

  // NOLINTBEGIN(google-explicit-constructor)
  // NOLINTEND(*)

How should Clang-Tidy behave when mixing check-specific `NOLINTBEGIN/END`s with globbed ones?



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:349
+      // Allow specifying a few checks with a glob expression.
+      GlobList Globs(ChecksStr);
+      if (!Globs.contains(CheckName))
----------------
What happens when `CheckStr` is empty?
How has Clang-Tidy treated `// NOLINT()` in the past? Does this patch change the behaviour? What //should // be the "right" behaviour?


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353
+      if (SuppressionIsSpecific)
+        *SuppressionIsSpecific = true;
     }
----------------
So when this function is called with args `Line = "NOLINTBEGIN(google*)"` and `CheckName = "google-explicit-constructor"`, it will return true with `*SuppressionIsSpecific = true`.
Should `*SuppressionIsSpecific = false` instead here?


================
Comment at: clang-tools-extra/docs/clang-tidy/index.rst:341
+    // Silence all warnings from the "google" module
+    Foo(bool param);  // NOLINT(google*)
+
----------------
To match the spacing in the above lines.


================
Comment at: clang-tools-extra/docs/clang-tidy/index.rst:344
+    // Silence all warnings, *except* the ones from the "google" module
+    Foo(bool param);  // NOLINT(*,-google*)
+
----------------



================
Comment at: clang-tools-extra/docs/clang-tidy/index.rst:347
+    // No warnings are suppressed, due to double negation
+    Foo(bool param);  // NOLINT(-google*)
   };
----------------
carlosgalvezp wrote:
> salman-javed-nz wrote:
> > Would anyone do this on purpose, or is this a user error?
> I don't see any use case here, no. I just thought to document it to clarify the double negation that's happening, in case a user gets confused and doesn't see the warning being suppressed.
> 
> Do you think it brings value or does more harm than good?



================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp:63
+
+class D6 { D6(int x); }; // NOLINT(*,-google*)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
----------------
How about changing `class D6` so that it violates 2 checks: a google one and a non-google one.
Then you can confirm whether the `*` in `NOLINT(*,-google*)` is working.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111208/new/

https://reviews.llvm.org/D111208