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