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

Salman Javed via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Oct 9 00:04:17 PDT 2021


salman-javed-nz added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353
+      if (SuppressionIsSpecific)
+        *SuppressionIsSpecific = true;
     }
----------------
salman-javed-nz wrote:
> carlosgalvezp wrote:
> > salman-javed-nz wrote:
> > > 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?
> > Good point. I honestly don't understand what SuppressionIsSpecific means and how it's used downstream, could you clarify?
> `SuppressionIsSpecific = true` means when a check is suppressed by name, i.e. `NOLINTBEGIN(check-name)`.
> 
> `SuppressionIsSpecific = false` means when `NOLINTBEGIN` <zero args> or `NOLINTBEGIN(*)` is used. My gut feeling is that It should probably be false when a glob is used too.
> 
> The point of this variable is so that a check-specific `BEGIN` can only be `END`ed with a check-specific END.
I think the behaviour has changed in this patch.

It used to return `*SuppressionIsSpecific = false` when `ChecksStr = "*"`. 
`"*"` is disabling all checks, not a specific check.

Now it returns `*SuppressionIsSpecific = true`. `Globs.contains(CheckName)` is true regardless of whether `ChecksStr = "*"` or `ChecksStr = "check-name"`, so it continues running onto line 353 and sets `*SuppressionIsSpecific = true` no matter what.


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

https://reviews.llvm.org/D111208



More information about the cfe-commits mailing list