salman-javed-nz added a comment. What should happen with the following code:
// NOLINTBEGIN(google*) // NOLINTEND(google-explicit-constructor) // NOLINTEND(google-runtime-operator) and this code: // NOLINTBEGIN(google-explicit-constructor) // NOLINTBEGIN(google-runtime-operator) // NOLINTEND(google*) At the moment, Clang-Tidy does not allow mixing "check-specific" `NOLINTBEGIN/END`s with "global" ones. See discussion in D108560#3020444 <https://reviews.llvm.org/D108560#3020444>. So the following is not a supported construct: // NOLINTBEGIN(google-explicit-constructor) // NOLINTEND(*) How should Clang-Tidy behave when mixing check-specific `NOLINTBEGIN/END`s with globbed ones? ================ 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)) ---------------- 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? ================ Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353 + if (SuppressionIsSpecific) + *SuppressionIsSpecific = true; } ---------------- 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? ================ Comment at: clang-tools-extra/docs/clang-tidy/index.rst:341 + // Silence all warnings from the "google" module + Foo(bool param); // NOLINT(google*) + ---------------- To match the spacing in the above lines. ================ Comment at: clang-tools-extra/docs/clang-tidy/index.rst:344 + // Silence all warnings, *except* the ones from the "google" module + Foo(bool param); // NOLINT(*,-google*) + ---------------- ================ Comment at: clang-tools-extra/docs/clang-tidy/index.rst:347 + // No warnings are suppressed, due to double negation + Foo(bool param); // NOLINT(-google*) }; ---------------- 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? ================ Comment at: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp:63 + +class D6 { D6(int x); }; // NOLINT(*,-google*) +// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit ---------------- How about changing `class D6` so that it violates 2 checks: a google one and a non-google one. Then you can confirm whether the `*` in `NOLINT(*,-google*)` is working. 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
