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

[Carlos Galvez via Phabricator via llvm-commits]

carlosgalvezp added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353
+      if (SuppressionIsSpecific)
+        *SuppressionIsSpecific = true;
     }
----------------
salman-javed-nz wrote:
> 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.
You are right! It's interesting however that this hasn't triggered any unit test error, so I wonder if this variable is needed at all then? I will have a deeper look at the code to get a better understanding and update the patch accordingly.


================
Comment at: clang-tools-extra/docs/clang-tidy/index.rst:347
+    // No warnings are suppressed, due to double negation
+    Foo(bool param);  // NOLINT(-google*)
   };
----------------
salman-javed-nz wrote:
> salman-javed-nz wrote:
> > 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?
> > 
> > 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?
> 
> I'd be happy with just the basic `+` glob functionality. The first thing I'd use it on is to suppress checks that are an alias of another check, e.g. `NOLINT(*no-malloc)` to cover both `hicpp-no-malloc` and `cppcoreguidelines-no-malloc`.
> 
> As for glob expressions that begin with a `-`... you get the functionality for free thanks to the `GlobList` class but it's not a feature I think I will need. I speak only for myself though. Maybe someone else here has a strong need for this?
> 
> Is `NOLINTBEGIN(-check)` equivalent to `NOLINTEND(check)`? It hursts my head even thinking about it.
> 
> Your unit test where you test `NOLINT(google*,-google*,google*)`, Clang-Tidy does the right thing in that situation, but I have to wonder why any user would want to add, remove, then add the same check group in the one expression in the first place? Should we even be entertaining this kind of use?
> Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)?

To me it's clear that it's not. Any NOLINTBEGIN(X) must have a matching NOLINTEND(X), regardless of whether X is a glob or not.

Totally agree that negative globs are probably not needed, the main use case is being able to suppress all aliases as you say.

I just thought it was neat to be able to reuse code in that way, the code is very clean and easy to read. Plus users are familiar with the syntax. If users want to abuse it that would be at their own maintenance expense - clang-tidy would just do what it was asked to do, without crashing or anything bad happening.

The same thing can be done when running the tool - you can run "--checks=google*,-google*" and I don't think clang-tidy has code to warn the user/prevent this from happening. Considering all these edge cases would perhaps complicate the design of the tool for no real use case? I.e. too defensive programming.

I added the unit test mostly to make sure clang-tidy doesn't crash or does something strange, it just does what the user instructed it to do. I thought such edge cases are encouraged in unit tests to increase coverage.

Would it be reasonable to keep negative globs in the implementation (to simplify maintenance, reuse code), but not document it in the public docs? Otherwise I'll need to refactor GlobList to be able to reuse the part of the code that consumes the positive globs. I don't think it makes sense to create a parallel implementation of that part (kind of what exists already now, before my patch).


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

https://reviews.llvm.org/D111208