carlosgalvezp added inline comments.
================ 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)) ---------------- 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. 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
