[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added a comment. > Unfortunately, precommit CI is pretty limited and extremely flakey, so the > only real way to know right now is to push and see how the bot farm likes it. > (There are a lot of bots we don't want to use for precommit CI because they > take a long time to run, us

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

aaron.ballman added a comment. In D111208#3073144 , @carlosgalvezp wrote: > Actually, `cpplint` is never compatible with `clang-tidy` regardless. The > following code: > > struct Foo > { > Foo(int) {} // NOLINT(google-explicit-constructor) >

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added a comment. A bit annoying about the build bot, fixed in a separate commit. Do you know if we can run these jobs pre-pushing? Seems rather unlikely to have them all pass the first time, especially for larger commits... Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

This revision was automatically updated to reflect the committed changes. Closed by commit rGbf6b0d16747f: [clang-tidy] Support globbing in NOLINT* expressions (authored by carlosgalvezp). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ ht

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added a comment. Actually, `cpplint` is never compatible with `clang-tidy` regardless. The following code: struct Foo { Foo(int) {} // NOLINT(google-explicit-constructor) }; Triggers: ../test.cpp:3: Unknown NOLINT error category: google-explicit-constructor [re

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, but if someone can verify we aren't introducing dueling diagnostics with cpplint before we land this, that would be appreciated. CHANGES SINCE LAST ACTION https://review

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 380693. carlosgalvezp marked 3 inline comments as done. carlosgalvezp added a comment. Addressed comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 Files: clang-tools-extra/clang-tidy/ClangT

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

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 Gl

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 379284. carlosgalvezp added a comment. Expand documentation to make it clear that globbing is supported for NOLINT, NOLINTNEXTLINE as well as NOLINTBEGIN/END. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

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

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 379097. carlosgalvezp marked an inline comment as done. carlosgalvezp added a comment. Addressed @salman-javed-nz comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 Files: clang-tools-extra/c

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/index.rst:314 +are ignored here, as they would effectively re-activate the warning, causing +confusion to the users. There's something about this "causing confusion to the user

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz 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)) carlosgalv

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added a comment. > Just an FYI, but we usually only ping when there's been about a week of no > traffic on the review Thanks, I'll keep it in mind for next time! Really appreciate your time reviewing :) I have updated the patch based on your feedback. CHANGES SINCE LAST ACTION

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

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.co

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 378913. carlosgalvezp marked an inline comment as done. carlosgalvezp added a comment. Remove support for negative globs. Keep unit tests to ensure that negative globs are ignored. Update documentation. CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

aaron.ballman added reviewers: njames93, hokein, whisperity. aaron.ballman added a comment. Herald added a subscriber: rnkovacs. In D111208#3054246 , @carlosgalvezp wrote: > Awesome, thanks a lot for the review, I really improved my understanding of > t

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added a comment. Awesome, thanks a lot for the review, I really improved my understanding of the tool :) @aaron.ballman Could you review? Otherwise could you point me to who should review? I tagged @alexfh as owner of clang-tidy but haven't really seen him around... Shouldn't the

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. Thanks for all the changes. You've addressed all my questions ad I don't have anything more to add. Let's wait for the reviewers to take a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:407-409 + if (!NolintBegins.empty() && + (NolintBegins.back().second == NolintBracket)) { +NolintBegins.pop_back(); carlosgalvezp

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:325 StringRef Line, size_t *FoundNolintIndex = nullptr, - bool *SuppressionIsSpecific = nullptr) { +

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 378507. carlosgalvezp marked 9 inline comments as done. carlosgalvezp added a comment. Fixed comments from @salman-javed-nz CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 Files: clang-tools-extra/

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:446 SuppressionErrors.emplace_back(Error.getValue()); -return false; } carlosgalvezp wrote: > I had to remove these "return false", otherwi

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. In D111208#3053660 , @carlosgalvezp wrote: > Now, the requirements for a match are stricter (and simpler), making the code > easier to reason about: any NOLINTBEGIN(X) must be matched by an identical > NOLINTEND(X), for

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:446 SuppressionErrors.emplace_back(Error.getValue()); -return false; } I had to remove these "return false", otherwise I would not get errors

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 378495. carlosgalvezp added a comment. Refactored NOLINTBEGIN/END slightly. Now, the requirements for a match are stricter (and simpler), making the code easier to reason about: any NOLINTBEGIN(X) must be matched by an identical NOLINTEND(X), for any X

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. In D111208#3053370 , @carlosgalvezp wrote: > By the way, is `NOLINTBEGIN/END` expected to work/give errors when the check > name is something else than a real check name? See for example: > > https://godbolt.org/z/b6cbTe

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added a comment. Thanks for the clarification, makes a lot of sense now! I'll see what I can do with that. By the way, is `NOLINTBEGIN/END` expected to work/give errors when the check name is something else than a real check name? See for example: https://godbolt.org/z/b6cbTeezs

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. In D111208#3053061 , @carlosgalvezp wrote: > Looking at the code, I see that you use `SuppressionIsSpecific` in order to > separate the errors into 2 error lists: `SpecificNolintBegins` and > `GlobalNolintBegins`. Howev

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 378427. carlosgalvezp marked 5 inline comments as done. carlosgalvezp added a comment. -Fixed formatting comments. -Added test for NOLINT(), NOLINTNEXTLINE() and NOLINTBEGIN() -Changed last test to more effectively test NOLINT(*,-google*) CHANGES SINCE

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

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.co

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added a comment. Looking at the code, I see that you use `SuppressionIsSpecific` in order to separate the errors into 2 error lists: `SpecificNolintBegins` and `GlobalNolintBegins`. However you don't do anything different with either list, so why do they need to be different lists

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353 + if (SuppressionIsSpecific) +*SuppressionIsSpecific = true; } salman-javed-nz wrote: > salman-javed-nz wrote: > > carlosgalvezp wr

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

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

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added a comment. In D111208#3050680 , @carlosgalvezp wrote: >> How should Clang-Tidy behave when mixing check-specific NOLINTBEGIN/ENDs >> with globbed ones? > > I would say keep current behavior - complain that the user wrote something

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:353 + if (SuppressionIsSpecific) +*SuppressionIsSpecific = true; } salman-javed-nz wrote: > So when this function is called with args `

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added a comment. > How should Clang-Tidy behave when mixing check-specific NOLINTBEGIN/ENDs with > globbed ones? I would say keep current behavior - complain that the user wrote something different in the BEGIN and in the END. I like keeping things simple and easy to read and rea

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

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)

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/index.rst:347 +// No warnings are suppressed, due to double negation +Foo(bool param); // NOLINT(-google*) }; salman-javed-nz wrote: > Would anyone do this on purpose,

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

salman-javed-nz added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/index.rst:347 +// No warnings are suppressed, due to double negation +Foo(bool param); // NOLINT(-google*) }; Would anyone do this on purpose, or is this a user error

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 377551. carlosgalvezp added a comment. Fixed comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tools-extra/docs/R

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:72 + multiple warnings in the same line. - Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress Clang-Tidy warnings over multiple lines. Please separate

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 377504. carlosgalvezp added a comment. Remove redundant "*" if branch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-to

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 377446. carlosgalvezp added a comment. Remove empty lines CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tools-extra/doc

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 377445. carlosgalvezp added a comment. Update comment in code. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tools-extr

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp updated this revision to Diff 377442. carlosgalvezp added a comment. Fixed formatting. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111208/new/ https://reviews.llvm.org/D111208 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tools-extra/docs

[PATCH] D111208: [clang-tidy] Support globbing in NOLINT* expressions

carlosgalvezp created this revision. carlosgalvezp added reviewers: aaron.ballman, alexfh. Herald added subscribers: arphaman, xazax.hun. carlosgalvezp requested review of this revision. Herald added a project: clang-tools-extra. To simplify suppressing multiple warnings, e.g. coming from check al