salman-javed-nz added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353 + if (SuppressionIsSpecific) + *SuppressionIsSpecific = true; } ---------------- salman-javed-nz wrote: > 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. I think the behaviour has changed in this patch. It used to return `*SuppressionIsSpecific = false` when `ChecksStr = "*"`. `"*"` is disabling all checks, not a specific check. Now it returns `*SuppressionIsSpecific = true`. `Globs.contains(CheckName)` is true regardless of whether `ChecksStr = "*"` or `ChecksStr = "check-name"`, so it continues running onto line 353 and sets `*SuppressionIsSpecific = true` no matter what. 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