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

[Carlos Galvez via Phabricator via llvm-commits]

carlosgalvezp marked an inline comment as done.
carlosgalvezp added inline comments.


================
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))
----------------
aaron.ballman wrote:
> carlosgalvezp wrote:
> > salman-javed-nz wrote:
> > > 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?
> > Very good question! Currently on master `// NOLINT()` will *not* suppress warnings. However `// NOLINT(` will. My patch shouldn't change existing behavior - an empty list of globs will return false when calling `contains`.
> > 
> > I'll add a unit test to cover this case!
> (All of this is a bit orthogonal to this patch, so no changes requested here.)
> 
> Naively, I would expect `NOLINT()` to be an error because it expects an argument and none was given. (I'd expect `NOLINT(` to also be an error because the syntax is malformed.)
Fully agree! If I find some time I can give it a try on a separate patch


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

https://reviews.llvm.org/D111208