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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:352
+      // Allow specifying a few checks with a glob expression, ignoring
+      // negative globs (which would effectively disable the suppression)
+      GlobList Globs(ChecksStr, /*KeepNegativeGlobs=*/false);
----------------



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:406
+      if (!NolintBegins.empty() &&
+          (NolintBegins.back().second == NolintChecksStr)) {
+        NolintBegins.pop_back();
----------------



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:455-456
+  for (const auto NolintBegin : NolintBegins) {
+    auto Error = createNolintError(Context, SM, NolintBegin.first, true);
     SuppressionErrors.emplace_back(Error);
   }
----------------



================
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:
> salman-javed-nz wrote:
> > 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.
> I'd gladly decouple clang-tidy and cpplint. Personally I don't find the benefit of running both, clang-tidy is way superior.
> 
> How important it is to maintain compatibility with the more funky aspects of cpplint's NOLINT behaviour, I'm not sure.

I think the important thing is for users who use both tools to not have a bad experience because clang-tidy is yelling at them about NOLINT comment syntax. Fixing a syntax issue to make clang-tidy happy only to break cpplint (and vice versa) would leave users stuck.

I don't have a copy of cpplint handy, so I'm not able to validate my own suggestions for diagnostics here. If you hear a suggestion that would lead us down the "warring diagnostics" path, please push back!


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

https://reviews.llvm.org/D111208