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

[Salman Javed via Phabricator via llvm-commits]

salman-javed-nz 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))
----------------
carlosgalvezp wrote:
> 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
The NOLINT syntax was borrowed from Google's cpplint.py. cpplint uses a regex search to find NOLINT expressions, but it's pretty naive, leading to quirks like `NOLINT(` being accepted as a valid expression. Clang-Tidy has inherited the same quirks by being interoperable with cpplint's syntax.

How important it is to maintain compatibility with the more funky aspects of cpplint's `NOLINT` behaviour, I'm not sure. The two programs' syntaxes aren't fully compatible nowadays anyway. To give an example, cpplint doesn't support multiple checks separated by commas on the same line. This glob feature will be another divergence. 

Just something to think about for next time.


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

https://reviews.llvm.org/D111208