carlosgalvezp marked an inline comment as done. 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)) ---------------- 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 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
