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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
