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

Salman Javed via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 8 05:15:27 PDT 2021


salman-javed-nz added a comment.

In D111208#3050680 <https://reviews.llvm.org/D111208#3050680>, @carlosgalvezp wrote:

>> How should Clang-Tidy behave when mixing check-specific NOLINTBEGIN/ENDs with globbed ones?
>
> I would say keep current behavior - complain that the user wrote something different in the BEGIN and in the END. I like keeping things simple and easy to read and reason about. I think adding smarter logic here would make clang-tidy's code more complicated, but also it would allow users to write more complicated NOLINT expressions that would be very hard to read and track.
>
> Honestly as a user I'm more than fine with one level of NOLINTBEGIN/END. Any more nesting than that causes me much more cognitive effort than it's worth.
>
> What do you think?

I think the current simple approach is the way to go. If and when someone comes back to us with a compelling case for why more complicated NOLINT syntax is needed, we can think about it then.



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353
+      if (SuppressionIsSpecific)
+        *SuppressionIsSpecific = true;
     }
----------------
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.


================
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:
> 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?


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

https://reviews.llvm.org/D111208



More information about the cfe-commits mailing list