[clang-tidy] Support globbing in NOLINT* expressions

[carlosgalvezp]

Summary

To simplify suppressing multiple warnings, e.g. coming from check aliases.

The code for globbing is the same as for enabling checks, so the semantics are identical. Naturally, the meaning is reversed since we are _disabling_ warnings in NOLINT expressions.

This patch also simplifies the logic of NOLINTBEGIN/END, removing the division between "global" vs "specific" suppressions. It's not clear whether // NOLINTBEGIN(google*) is global or specific, for example.

Instead, keep it easy: NOLINTBEGIN(X) must have a matching NOLINTEND(X), for any X. This enforcement leads to easier to read code on the user side.

[carlosgalvezp]

Fixed formatting.

[carlosgalvezp]

Update comment in code.

[carlosgalvezp]

Remove empty lines

[carlosgalvezp]

Remove redundant "*" if branch.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

73

Please separate with newline.

[carlosgalvezp]

Fixed comments.

[salman-javed-nz]

clang-tools-extra/docs/clang-tidy/index.rst

347

Would anyone do this on purpose, or is this a user error?

[carlosgalvezp]

clang-tools-extra/docs/clang-tidy/index.rst

347

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?

[salman-javed-nz]

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/ENDs with "global" ones. See discussion in 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/ENDs with globbed ones?

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

350

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?

354

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?

clang-tools-extra/docs/clang-tidy/index.rst

341

To match the spacing in the above lines.

344

347

clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp

66

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.

[carlosgalvezp]

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 reason about. I think adding smarter logic here would make clang-tidy's code more complicated, but also it would allow users to write more complicated NOLINT expressions that would be very hard to read and track.

Honestly as a user I'm more than fine with one level of NOLINTBEGIN/END. Any more nesting than that causes me much more cognitive effort than it's worth.

What do you think?

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

354

Good point. I honestly don't understand what SuppressionIsSpecific means and how it's used downstream, could you clarify?

[salman-javed-nz]

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 different in the BEGIN and in the END. I like keeping things simple and easy to read and reason about. I think adding smarter logic here would make clang-tidy's code more complicated, but also it would allow users to write more complicated NOLINT expressions that would be very hard to read and track.

Honestly as a user I'm more than fine with one level of NOLINTBEGIN/END. Any more nesting than that causes me much more cognitive effort than it's worth.

What do you think?

I think the current simple approach is the way to go. If and when someone comes back to us with a compelling case for why more complicated NOLINT syntax is needed, we can think about it then.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

354

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 ENDed with a check-specific END.

clang-tools-extra/docs/clang-tidy/index.rst

347

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?

I'd be happy with just the basic + glob functionality. The first thing I'd use it on is to suppress checks that are an alias of another check, e.g. NOLINT(*no-malloc) to cover both hicpp-no-malloc and cppcoreguidelines-no-malloc.

As for glob expressions that begin with a -... you get the functionality for free thanks to the GlobList class but it's not a feature I think I will need. I speak only for myself though. Maybe someone else here has a strong need for this?

Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)? It hursts my head even thinking about it.

Your unit test where you test NOLINT(google*,-google*,google*), Clang-Tidy does the right thing in that situation, but I have to wonder why any user would want to add, remove, then add the same check group in the one expression in the first place? Should we even be entertaining this kind of use?

[salman-javed-nz]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

354

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.

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

354

You are right! It's interesting however that this hasn't triggered any unit test error, so I wonder if this variable is needed at all then? I will have a deeper look at the code to get a better understanding and update the patch accordingly.

clang-tools-extra/docs/clang-tidy/index.rst

347

Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)?

To me it's clear that it's not. Any NOLINTBEGIN(X) must have a matching NOLINTEND(X), regardless of whether X is a glob or not.

Totally agree that negative globs are probably not needed, the main use case is being able to suppress all aliases as you say.

I just thought it was neat to be able to reuse code in that way, the code is very clean and easy to read. Plus users are familiar with the syntax. If users want to abuse it that would be at their own maintenance expense - clang-tidy would just do what it was asked to do, without crashing or anything bad happening.

The same thing can be done when running the tool - you can run "--checks=google*,-google*" and I don't think clang-tidy has code to warn the user/prevent this from happening. Considering all these edge cases would perhaps complicate the design of the tool for no real use case? I.e. too defensive programming.

I added the unit test mostly to make sure clang-tidy doesn't crash or does something strange, it just does what the user instructed it to do. I thought such edge cases are encouraged in unit tests to increase coverage.

Would it be reasonable to keep negative globs in the implementation (to simplify maintenance, reuse code), but not document it in the public docs? Otherwise I'll need to refactor GlobList to be able to reuse the part of the code that consumes the positive globs. I don't think it makes sense to create a parallel implementation of that part (kind of what exists already now, before my patch).

[carlosgalvezp]

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?

Here checking that a combined list is empty would be equivalent:

bool WithinNolintBegin =
    !SpecificNolintBegins.empty() || !GlobalNolintBegins.empty();

And here, you are running identical code for both lists:

for (const auto NolintBegin : SpecificNolintBegins) {
  auto Error = createNolintError(Context, SM, NolintBegin, true);
  SuppressionErrors.emplace_back(Error);
}
for (const auto NolintBegin : GlobalNolintBegins) {
  auto Error = createNolintError(Context, SM, NolintBegin, true);
  SuppressionErrors.emplace_back(Error);
}

And then these lists are not used any further than the scope of the function where they are declared. So to me it feels like they could be combined, and this logic of SuppressionIsSpecific be removed. Let me know if I'm missing something obvious!

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

350

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!

[carlosgalvezp]

-Fixed formatting comments.
-Added test for NOLINT(), NOLINTNEXTLINE() and NOLINTBEGIN()
-Changed last test to more effectively test NOLINT(*,-google*)

[salman-javed-nz]

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. However you don't do anything different with either list, so why do they need to be different lists?

Here checking that a combined list is empty would be equivalent:

bool WithinNolintBegin =
    !SpecificNolintBegins.empty() || !GlobalNolintBegins.empty();

And here, you are running identical code for both lists:

for (const auto NolintBegin : SpecificNolintBegins) {
  auto Error = createNolintError(Context, SM, NolintBegin, true);
  SuppressionErrors.emplace_back(Error);
}
for (const auto NolintBegin : GlobalNolintBegins) {
  auto Error = createNolintError(Context, SM, NolintBegin, true);
  SuppressionErrors.emplace_back(Error);
}

And then these lists are not used any further than the scope of the function where they are declared. So to me it feels like they could be combined, and this logic of SuppressionIsSpecific be removed. Let me know if I'm missing something obvious!

tallyNolintBegins() goes through every line, and whenever it sees a NOLINTBEGIN, it pushes its SourceLocation onto a stack. When it sees a NOLINTEND, it pops one entry off the stack. At the end of the file, all entries on the stack should have been popped off - if not, it's a unmatched NOLINTBEGIN/END expression error.

NOLINTBEGIN(check-name) expressions are pushed onto the SpecificNolintBegins list, while NOLINTBEGIN & NOLINTBEGIN(*) expressions are pushed onto the GlobalNolintBegins list. The reason for the two lists is so that NOLINTBEGIN(check-name) cannot be popped off by NOLINTEND or NOLINTEND(*); and NOLINTBEGIN and NOLINTBEGIN(*) cannot be popped off by a NOLINTEND(check-name).

See this line in tallyNolintBegins() where it figures out which list to use for the NOLINTBEGIN/END expression currently being evaluated...

auto List = [&]() -> SmallVector<SourceLocation> * {
  return SuppressionIsSpecific ? &SpecificNolintBegins : &GlobalNolintBegins;
};

I'm sure there is an alternate implementation which achieves the same thing with just one list object... however the list will probably need to store more information about the NOLINTBEGIN expression than just its SourceLocation to be able to achieve the same goals though... probably a list of pair<SourceLocation Location, StringRef CheckName>, otherwise you can't match a NOLINTEND to its corresponding NOLINTBEGIN. If you want to take a crack at simplifying the logic here, all improvements are appreciated.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

354

You are right! It's interesting however that this hasn't triggered any unit test error

That's because the unit tests are missing a test case that checks this specific combination of BEGIN/END expressions. The tests predominantly use NOLINTBEGIN <zero args>, not NOLINTBEGIN(*).
It would be great if this shortcoming in the test coverage could be plugged in this patch.

nolintbeginend-begin-global-end-specific.cpp checks:

// NOLINTBEGIN
class A { A(int i); };
// NOLINTEND(google-explicit-constructor)

nolintbeginend-begin-specific-end-global.cpp checks:

// NOLINTBEGIN(google-explicit-constructor)
class A { A(int i); };
// NOLINTEND

These 2 files could be used as the basis to create tests for NOLINTBEGIN(*) / NOLINTEND(*).

clang-tools-extra/docs/clang-tidy/index.rst

347

Is NOLINTBEGIN(-check) equivalent to NOLINTEND(check)?

To me it's clear that it's not. Any NOLINTBEGIN(X) must have a matching NOLINTEND(X), regardless of whether X is a glob or not.

Totally agree that negative globs are probably not needed, the main use case is being able to suppress all aliases as you say.

I just thought it was neat to be able to reuse code in that way, the code is very clean and easy to read. Plus users are familiar with the syntax. If users want to abuse it that would be at their own maintenance expense - clang-tidy would just do what it was asked to do, without crashing or anything bad happening.

The same thing can be done when running the tool - you can run "--checks=google*,-google*" and I don't think clang-tidy has code to warn the user/prevent this from happening. Considering all these edge cases would perhaps complicate the design of the tool for no real use case? I.e. too defensive programming.

I added the unit test mostly to make sure clang-tidy doesn't crash or does something strange, it just does what the user instructed it to do. I thought such edge cases are encouraged in unit tests to increase coverage.

I see all your points about reusing the glob functionality. As long as this functionality does not let a user shoot themselves in the foot, I don't see a problem with it.
A large proportion of the review of my NOLINTBEGIN/NOLINTEND patch was spent going over all the ways a user could misuse the functionality. The concern was mainly that we didn't want a user to unwittingly disable checks for the entire file because of a stray NOLINTBEGIN without an NOLINTEND marker.
The worst I can see a confused user do with this new glob feature is use NOLINT(-check*) which as you have already explained is a double negative and shows the warnings anyway, so no harm done.

I find it funny that master allows NOLINT( with no closing bracket go though as a NOLINT when it's clearly a sign of user error. Clang-Tidy could be more proactive in alerting the user to these mistakes, but I get there's a balance between code complexity and harm reduction. This quirk is not directly related to your patch so that's a story to continue on another day.

Would it be reasonable to keep negative globs in the implementation (to simplify maintenance, reuse code), but not document it in the public docs? Otherwise I'll need to refactor GlobList to be able to reuse the part of the code that consumes the positive globs. I don't think it makes sense to create a parallel implementation of that part (kind of what exists already now, before my patch).

I'm only here to provide some insight about code I recently wrote. Let's wait for the actual reviewers who have a better sense of the direction of this project to say how things should be.

[carlosgalvezp]

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

I would expect to get a warning that an END was found that didn't match a BEGIN.

[salman-javed-nz]

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/b6cbTeezs

I would expect to get a warning that an END was found that didn't match a BEGIN.

The functions that we have been discussing in this patch only run when Clang-Tidy has detected a check violation and is about to print a warning about it. The NOLINT comment checks happen at the last minute. Since foo and bar are not real checks, Clang-Tidy won't find any violations, and so the NOLINT checks won't get the opportunity to run.

[carlosgalvezp]

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.

Added additional tests.

@salman-javed-nz let me know what you think!

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

446

I had to remove these "return false", otherwise I would not get errors regarding "Found NOLINTBEGIN that was unmatched by a NOLINTEND". Not really sure why they were there in the first place.

All unit tests pass with this patch.

[salman-javed-nz]

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 any X.

Thanks! Simplifying it right down looks to me like the right thing to do.

In case you find it hard to follow my suggested changes in Phabricator's interface, I have copied these suggestions into this attachment for convenience of reading:

changes.cpp4 KBDownload

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

325

This feels like a more intuitive argument name to me, plus it aligns with the local variable ChecksStr below. What do you think?

343–355

I think this should work the same without the additional index variable and substring.

344

This is just a copy of BracketIndex. Would it be better to consolidate the two?

347–352

It gets a bit hard to follow with BracketIndex and BracketStartIndex interspersed on these lines, and a + 1 in one substring but not in the other substring.
Also aren't both substrings pretty much the same thing?

394

To align with the name ChecksStr used in the other function(s).

404–407

It's not necessarily the last element of NolintBegins that you need to pop_back().

In the case where you have overlapping NOLINTBEGIN regions...

// NOLINTBEGIN(A)  <-- push A
// NOLINTBEGIN(B) <-- push B
// NOLINTEND(A) <-- pop A
// NOLINTEND(B) <-- pop B

Therefore you need to search through all of the elements, not just the last one.

clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

57

Since C6 & C7 have been removed you could renumber C8 to C6. Once this feature is in master I don't see this test file changing again for a long time.

[salman-javed-nz]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

446

The reason for the return false was because the file is already determined to have a unmatched NOLINTBEGIN, so it's FUBAR and probably not worth checking the remainder of anyway. I don't mind either way.

[carlosgalvezp]

Fixed comments from @salman-javed-nz

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

325

Sure. What about FoundNolintChecksStr, for consistency with FoundNolintIndex?

347–352

I wanted "FoundCheckStr" to contain also the brackets, so that NOLINT( is different than NOLINT(). However I realize now that they are anyway treated differently (as per previous discussion) so I'll remove this and keep it simple.

404–407

Thanks, that's right. However that's a problem that already exists on master, right? It's pushing from the back anyway? We should probably have a unit test for this.

Would it make sense to leave that for a separate patch? Seems like the scope is growing and I'd like to keep the change as small as possible.

446

I see. In the test case that I added, I got prints that NOLINTEND is unmatched by a BEGIN, but I didn't get a message that NOLINTBEGIN is unmatched by an END, which I did get in a separate unit test, so I thought that's intended behavior.

[salman-javed-nz]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

404–407

Thanks, that's right. However that's a problem that already exists on master, right? It's pushing from the back anyway?

Actually yes, you're right. Disregard my previous comment. I was thinking of a scenario that isn't a problem.
It's all looking good to me now.

[salman-javed-nz]

Thanks for all the changes. You've addressed all my questions and I don't have anything more to add. Let's wait for the reviewers to take a look.

[carlosgalvezp]

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 there be multiple owners, so that there's not a "single point of failure"?

[aaron.ballman]

In D111208#3054246, @carlosgalvezp wrote:

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 there be multiple owners, so that there's not a "single point of failure"?

I'm happy to review (I'll add a few more folks to the list though). Just an FYI, but we usually only ping when there's been about a week of no traffic on the review. Reviewers have a fairly heavy workload and we've settled on a week between pings as being the happy medium.

In D111208#3053370, @carlosgalvezp wrote:

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

I would expect to get a warning that an END was found that didn't match a BEGIN.

If the tags don't match (so there's an open for a check but never a close for the check), then we should diagnose. However, we should *not* diagnose unknown check names (because the user may be suppressing checks that clang-tidy doesn't know about, such as ones in another static analysis tool run over the same code). However, given that we don't run the NOLINT pass unless there are diagnostics to emit, I think it's fine to not diagnose a mismatch between unknown tags.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

350

(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.)

clang-tools-extra/docs/clang-tidy/index.rst

347

I see all your points about reusing the glob functionality. As long as this functionality does not let a user shoot themselves in the foot, I don't see a problem with it.

Likewise; code reuse is a good thing when it works in our favor.

The worst I can see a confused user do with this new glob feature is use NOLINT(-check*) which as you have already explained is a double negative and shows the warnings anyway, so no harm done.

I have to think on this more, but my initial reaction is that it's a bit scary because the check names themselves have dashes within them already, but those dashes don't mean negation. So I worry about this being confusing:

// NOLINT(misc-whatever-awesome)
// Oops, we didn't notice that there's also bugprone-whatever-awesome, 
// so let's try to suppress them all.

// NOLINT(-whatever-awesome)
// Oops. that actual disables the check because we didn't know to add the star. But because it's in a NOLINT, that's a double negation, so we've reenabled the check. So the user will still get diagnostics, but likely not understand why.

// NOLINT(*-whatever-awesome)
// This was what we meant to write.

I don't think allowing users to do double-negations in NOLINT comments adds a lot of value. Would it be reasonable to pass in a flag to GlobList to disable negative glob handling so we can still reuse the logic but not have to enable a weird feature that users may accidentally run into?

[carlosgalvezp]

Remove support for negative globs. Keep unit tests to ensure that negative globs are ignored. Update documentation.

[carlosgalvezp]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

350

Fully agree! If I find some time I can give it a try on a separate patch

[carlosgalvezp]

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.

[salman-javed-nz]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

350

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.

[salman-javed-nz]

clang-tools-extra/docs/clang-tidy/index.rst

314

There's something about this "causing confusion to the users" that my mind latches on to. I'm not sure what my problem with it is. Perhaps the sentence will read better if you just remove the 2nd half?

[carlosgalvezp]

Addressed @salman-javed-nz comment.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

350

I'd gladly decouple clang-tidy and cpplint. Personally I don't find the benefit of running both, clang-tidy is way superior.

[carlosgalvezp]

Expand documentation to make it clear that globbing is supported for NOLINT, NOLINTNEXTLINE as well as NOLINTBEGIN/END.

[aaron.ballman]

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

350

How important it is to maintain compatibility with the more funky aspects of cpplint's NOLINT behaviour, I'm not sure.

I think the important thing is for users who use both tools to not have a bad experience because clang-tidy is yelling at them about NOLINT comment syntax. Fixing a syntax issue to make clang-tidy happy only to break cpplint (and vice versa) would leave users stuck.

I don't have a copy of cpplint handy, so I'm not able to validate my own suggestions for diagnostics here. If you hear a suggestion that would lead us down the "warring diagnostics" path, please push back!

352

405

454–455

[carlosgalvezp]

Addressed comments.

[aaron.ballman]

LGTM, but if someone can verify we aren't introducing dueling diagnostics with cpplint before we land this, that would be appreciated.

[carlosgalvezp]

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  [readability/nolint] [5]

So one has to disable the diagnostic anyway via --filter=-readability/nolint. I've tested using globs and cpplint works fine so long the diagnostic is suppressed. I.e. this patch doesn't change things in that respect.

I go ahead and merge if you are happy then!

[carlosgalvezp]

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

[aaron.ballman]

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)
};

Triggers:

../test.cpp:3:  Unknown NOLINT error category: google-explicit-constructor  [readability/nolint] [5]

So one has to disable the diagnostic anyway via --filter=-readability/nolint. I've tested using globs and cpplint works fine so long the diagnostic is suppressed. I.e. this patch doesn't change things in that respect.

I go ahead and merge if you are happy then!

Oh, that's a bummer. :-( Oh well, your patch doesn't make that any worse.

In D111208#3073160, @carlosgalvezp wrote:

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

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, use experimental or flakey hardware, etc.)

[carlosgalvezp]

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, use experimental or flakey hardware, etc.)

I see, I'll make sure to plan for watching the build bots post-merge then. Thanks!

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

 #include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Regex.h"
 #include <tuple>
+#include <utility>
 #include <vector>
 using namespace clang;
 using namespace tidy;
 
 namespace {
 class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
 public:
   ClangTidyDiagnosticRenderer(const LangOptions &LangOpts,
if (!Errors.empty()) {
     }
   }
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
 
 static bool isNOLINTFound(StringRef NolintDirectiveText, StringRef CheckName,
                           StringRef Line, size_t *FoundNolintIndex = nullptr,
-                          bool *SuppressionIsSpecific = nullptr) {
+                          StringRef *FoundNolintChecksStr = nullptr) {
   if (FoundNolintIndex)
     *FoundNolintIndex = StringRef::npos;
-  if (SuppressionIsSpecific)
-    *SuppressionIsSpecific = false;
+  if (FoundNolintChecksStr)
+    *FoundNolintChecksStr = StringRef();
 
   size_t NolintIndex = Line.find(NolintDirectiveText);
   if (NolintIndex == StringRef::npos)
     return false;
 
   size_t BracketIndex = NolintIndex + NolintDirectiveText.size();
   if (BracketIndex < Line.size() && isalnum(Line[BracketIndex])) {
     // Reject this search result, otherwise it will cause false positives when
     // NOLINT is found as a substring of NOLINT(NEXTLINE/BEGIN/END).
     return false;
   }
 
   // Check if specific checks are specified in brackets.
   if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
     ++BracketIndex;
     const size_t BracketEndIndex = Line.find(')', BracketIndex);
     if (BracketEndIndex != StringRef::npos) {
       StringRef ChecksStr =
           Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
-      // Allow disabling all the checks with "*".
-      if (ChecksStr != "*") {
-        // Allow specifying a few check names, delimited with comma.
-        SmallVector<StringRef, 1> Checks;
-        ChecksStr.split(Checks, ',', -1, false);
-        llvm::transform(Checks, Checks.begin(),
-                        [](StringRef S) { return S.trim(); });
-        if (llvm::find(Checks, CheckName) == Checks.end())
-          return false;
-        if (SuppressionIsSpecific)
-          *SuppressionIsSpecific = true;
-      }
+      if (FoundNolintChecksStr)
+        *FoundNolintChecksStr = ChecksStr;
+      // Allow specifying a few checks with a glob expression.
+      GlobList Globs(ChecksStr);
+      if (!Globs.contains(CheckName))
     }
   }
 
   if (FoundNolintIndex)
     *FoundNolintIndex = NolintIndex;
 
   return true;
 }
 
StringRef Message =
           ? "unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINTEND' "
             "comment"
           : "unmatched 'NOLINTEND' comment without a previous 'NOLINTBEGIN' "
             "comment";
   Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
   return Error;
 }
 
-static Optional<ClangTidyError>
-tallyNolintBegins(const ClangTidyContext &Context, const SourceManager &SM,
-                  StringRef CheckName, SmallVector<StringRef> Lines,
-                  SourceLocation LinesLoc,
-                  SmallVector<SourceLocation> &SpecificNolintBegins,
-                  SmallVector<SourceLocation> &GlobalNolintBegins) {
-  // Keep a running total of how many NOLINT(BEGIN...END) blocks are active.
+static Optional<ClangTidyError> tallyNolintBegins(
+    const ClangTidyContext &Context, const SourceManager &SM,
+    StringRef CheckName, SmallVector<StringRef> Lines, SourceLocation LinesLoc,
+    SmallVector<std::pair<SourceLocation, StringRef>> &NolintBegins) {
+  // Keep a running total of how many NOLINT(BEGIN...END) blocks are active, as
+  // well as the bracket expression (if any) that was used in the NOLINT
+  // expression.
   size_t NolintIndex;
-  bool SuppressionIsSpecific;
-  auto List = [&]() -> SmallVector<SourceLocation> * {
-    return SuppressionIsSpecific ? &SpecificNolintBegins : &GlobalNolintBegins;
-  };
+  StringRef NolintChecksStr;
   for (const auto &Line : Lines) {
     if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, &NolintIndex,
-                      &SuppressionIsSpecific)) {
+                      &NolintChecksStr)) {
       // Check if a new block is being started.
-      List()->emplace_back(LinesLoc.getLocWithOffset(NolintIndex));
+      NolintBegins.emplace_back(std::make_pair(
+          LinesLoc.getLocWithOffset(NolintIndex), NolintChecksStr));
     } else if (isNOLINTFound("NOLINTEND", CheckName, Line, &NolintIndex,
-                             &SuppressionIsSpecific)) {
+                             &NolintChecksStr)) {
       // Check if the previous block is being closed.
-      if (!List()->empty()) {
-        List()->pop_back();
+      if (!NolintBegins.empty() &&
+          (NolintBegins.back().second == NolintChecksStr)) {
+        NolintBegins.pop_back();
       } else {
         // Trying to close a nonexistent block. Return a diagnostic about this
         // misuse that can be displayed along with the original clang-tidy check
         // that the user was attempting to suppress.
         return createNolintError(Context, SM,
                                  LinesLoc.getLocWithOffset(NolintIndex), false);
       }
     }
     // Advance source location to the next line.
     LinesLoc = LinesLoc.getLocWithOffset(Line.size() + sizeof('\n'));
   }
   return None; // All NOLINT(BEGIN/END) use has been consistent so far.
 }
 
 static bool
 lineIsWithinNolintBegin(const ClangTidyContext &Context,
                         SmallVectorImpl<ClangTidyError> &SuppressionErrors,
                         const SourceManager &SM, SourceLocation Loc,
                         StringRef CheckName, StringRef TextBeforeDiag,
                         StringRef TextAfterDiag) {
   Loc = SM.getExpansionRange(Loc).getBegin();
   SourceLocation FileStartLoc = SM.getLocForStartOfFile(SM.getFileID(Loc));
+  SmallVector<std::pair<SourceLocation, StringRef>> NolintBegins;
 
   // Check if there's an open NOLINT(BEGIN...END) block on the previous lines.
   SmallVector<StringRef> PrevLines;
   TextBeforeDiag.split(PrevLines, '\n');
-  SmallVector<SourceLocation> SpecificNolintBegins;
-  SmallVector<SourceLocation> GlobalNolintBegins;
-  auto Error =
-      tallyNolintBegins(Context, SM, CheckName, PrevLines, FileStartLoc,
-                        SpecificNolintBegins, GlobalNolintBegins);
+  auto Error = tallyNolintBegins(Context, SM, CheckName, PrevLines,
+                                 FileStartLoc, NolintBegins);
   if (Error) {
     SuppressionErrors.emplace_back(Error.getValue());
-    return false;
   }
-  bool WithinNolintBegin =
-      !SpecificNolintBegins.empty() || !GlobalNolintBegins.empty();
+  bool WithinNolintBegin = !NolintBegins.empty();
 
   // Check that every block is terminated correctly on the following lines.
   SmallVector<StringRef> FollowingLines;
   TextAfterDiag.split(FollowingLines, '\n');
   Error = tallyNolintBegins(Context, SM, CheckName, FollowingLines, Loc,
-                            SpecificNolintBegins, GlobalNolintBegins);
+                            NolintBegins);
   if (Error) {
     SuppressionErrors.emplace_back(Error.getValue());
-    return false;
   }
 
   // The following blocks were never closed. Return diagnostics for each
   // instance that can be displayed along with the original clang-tidy check
   // that the user was attempting to suppress.
-  for (const auto NolintBegin : SpecificNolintBegins) {
-    auto Error = createNolintError(Context, SM, NolintBegin, true);
-    SuppressionErrors.emplace_back(Error);
-  }
-  for (const auto NolintBegin : GlobalNolintBegins) {
-    auto Error = createNolintError(Context, SM, NolintBegin, true);
+  for (const auto NolintBegin : NolintBegins) {
+    auto Error = createNolintError(Context, SM, NolintBegin.first, true);
     SuppressionErrors.emplace_back(Error);
   }
 
   return WithinNolintBegin && SuppressionErrors.empty();
 }
 
 static bool
 lineIsMarkedWithNOLINT(const ClangTidyContext &Context,
                        SmallVectorImpl<ClangTidyError> &SuppressionErrors,

clang-tools-extra/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- Added support for globbing in `NOLINT*` expressions, to simplify suppressing
+  multiple warnings in the same line.
+
 - Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress
   Clang-Tidy warnings over multiple lines.
 
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-suspicious-memory-comparison
   <clang-tidy/checks/bugprone-suspicious-memory-comparison>` check.
 

clang-tools-extra/docs/clang-tidy/index.rst

--fix                          -
                                      Apply suggested fixes. Without -fix-errors
                                      clang-tidy will bail out if any compilation
                                      errors were found.
     --fix-errors                   -
                                      Apply suggested fixes even if compilation
                                      errors were found. If compiler errors have
                                      attached fix-its, clang-tidy will apply them as
                                      well.
-    --fix-notes                    - 
-                                     If a warning has no fix, but a single fix can 
-                                     be found through an associated diagnostic note, 
-                                     apply the fix. 
-                                     Specifying this flag will implicitly enable the 
+    --fix-notes                    -
+                                     If a warning has no fix, but a single fix can
+                                     be found through an associated diagnostic note,
+                                     apply the fix.
+                                     Specifying this flag will implicitly enable the
                                      '--fix' flag.
     --format-style=<string>        -
                                      Style for formatting code around applied fixes:
                                        - 'none' (default) turns off formatting
                                        - 'file' (literally 'file', not a placeholder)
                                          uses .clang-format file in the closest parent
                                          directory
                                        - '{ <json> }' specifies options inline, e.g.

 language construct; it applies to the line of code it is on). If introducing the
 comment in the same line would change the formatting in an undesired way, the
 ``NOLINTNEXTLINE`` comment allows to suppress clang-tidy warnings on the *next
 line*. The ``NOLINTBEGIN`` and ``NOLINTEND`` comments allow suppressing
 clang-tidy warnings on *multiple lines* (affecting all lines between the two
 comments).
 
 All comments can be followed by an optional list of check names in parentheses
-(see below for the formal syntax).
+(see below for the formal syntax). The list of check names supports globbing,
+with the same format and semantics as for enabling checks. Note that adding
+a dash, e.g. `NOLINT(-check-name)`, is a double negation ("do *not* suppress
+`check-name`"), so the warning is not expected to be suppressed.
 
 For example:
 
 .. code-block:: c++
 
   class Foo {
     // Suppress all the diagnostics for the line
     Foo(int param); // NOLINT
 
     // Consider explaining the motivation to suppress the warning.
     Foo(char param); // NOLINT: Allow implicit conversion from `char`, because <some valid reason>.
 
     // Silence only the specified checks for the line
     Foo(double param); // NOLINT(google-explicit-constructor, google-runtime-int)
 
     // Silence only the specified diagnostics for the next line
     // NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
     Foo(bool param);
 
     // Silence only the specified checks for all lines between the BEGIN and END
     // NOLINTBEGIN(google-explicit-constructor, google-runtime-int)
     Foo(short param);
     Foo(long param);
     // NOLINTEND(google-explicit-constructor, google-runtime-int)
+
+    // Silence all warnings from the "google" module
+    Foo(bool param); // NOLINT(google*)
+
+    // Silence all warnings, *except* the ones from the "google" module
+    Foo(bool param); // NOLINT(*,-google*)
+
+    // No warnings are suppressed, due to double negation
+    Foo(bool param); // NOLINT(-google*)
   };
 
 The formal syntax of ``NOLINT``, ``NOLINTNEXTLINE``, and ``NOLINTBEGIN`` ...
 ``NOLINTEND`` is the following:
 
 .. parsed-literal::
 
   lint-comment:

clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp

 class A { A(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 class B { B(int i); }; // NOLINT
 
 class C { C(int i); }; // NOLINT(for-some-other-check)
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
-class C1 { C1(int i); }; // NOLINT(*)
+class C1 { C1(int i); }; // NOLINT()
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+class C2 { C2(int i); }; // NOLINT(*)
 
-class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+class C3 { C3(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
 
-class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+class C4 { C4(int i); }; // NOLINT(google-explicit-constructor)
 
-class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+class C5 { C5(int i); }; // NOLINT(some-check, google-explicit-constructor)
 
-class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
+class C6 { C6(int i); }; // NOLINT without-brackets-skip-all, another-check
 
-class C6 { C6(int i); }; // NOLINTNEXTLINE doesn't get misconstrued as a NOLINT
+class C7 { C7(int i); }; // NOLINTNEXTLINE doesn't get misconstrued as a NOLINT
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
 
 void f() {
   int i;
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: unused variable 'i' [clang-diagnostic-unused-variable]
   int j; // NOLINT
   int k; // NOLINT(clang-diagnostic-unused-variable)
 }
 
 #define MACRO(X) class X { X(int i); };
 MACRO(D)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 MACRO(E) // NOLINT
 
 #define MACRO_NOARG class F { F(int i); };
 MACRO_NOARG // NOLINT
 
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 13 warnings (13 NOLINT)
+class D1 { D1(int x); }; // NOLINT(google*)
+class D2 { D2(int x); }; // NOLINT(*explicit-constructor)
+class D3 { D3(int x); }; // NOLINT(*explicit*)
+
+class D4 { D4(int x); }; // NOLINT(google*,-google*)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+class D5 { D5(int x); }; // NOLINT(google*,-google*,google*)
+
+class D6 { D6(int x); void f() { int i; } };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:38: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+
+class D7 { D7(int x); void f() { int i; } }; // NOLINT(*,-google*)
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// CHECK-MESSAGES: Suppressed 18 warnings (18 NOLINT)

clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp

 // RUN: not clang-tidy %s --checks='-*,google-explicit-constructor' 2>&1 | FileCheck %s
 
 // NOLINTBEGIN
 class A { A(int i); };
 // NOLINTEND(google-explicit-constructor)
 
 // Note: the expected output has been split over several lines so that clang-tidy
 //       does not see the "no lint" suppression comment and mistakenly assume it
 //       is meant for itself.
 // CHECK: :[[@LINE-6]]:11: warning: single-argument constructors must be marked explicit
 // CHECK: :[[@LINE-6]]:4: error: unmatched 'NOLIN
 // CHECK: TEND' comment without a previous 'NOLIN
 // CHECK: TBEGIN' comment [clang-tidy-nolint]
+
+// NOLINTBEGIN
+class B { B(int i); };
+// NOLINTEND(*)
+
+// Note: the expected output has been split over several lines so that clang-tidy
+//       does not see the "no lint" suppression comment and mistakenly assume it
+//       is meant for itself.
+// CHECK: :[[@LINE-6]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-6]]:4: error: unmatched 'NOLIN
+// CHECK: TEND' comment without a previous 'NOLIN
+// CHECK: TBEGIN' comment [clang-tidy-nolint]

clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.cpp

 // RUN: not clang-tidy %s --checks="-*,google-explicit-constructor" 2>&1 | FileCheck %s
 
 // NOLINTBEGIN(google-explicit-constructor)
 class A { A(int i); };
 // NOLINTEND
 
 // Note: the expected output has been split over several lines so that clang-tidy
 //       does not see the "no lint" suppression comment and mistakenly assume it
 //       is meant for itself.
 // CHECK: :[[@LINE-6]]:11: warning: single-argument constructors must be marked explicit
 // CHECK: :[[@LINE-6]]:4: error: unmatched 'NOLIN
 // CHECK: TEND' comment without a previous 'NOLIN
 // CHECK: TBEGIN' comment [clang-tidy-nolint]
+
+// NOLINTBEGIN(*)
+class B { B(int i); };
+// NOLINTEND
+
+// Note: the expected output has been split over several lines so that clang-tidy
+//       does not see the "no lint" suppression comment and mistakenly assume it
+//       is meant for itself.
+// CHECK: :[[@LINE-6]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-6]]:4: error: unmatched 'NOLIN
+// CHECK: TEND' comment without a previous 'NOLIN
+// CHECK: TBEGIN' comment [clang-tidy-nolint]

clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-check-names.cpp

 // CHECK: :[[@LINE-8]]:4: error: unmatched 'NOLIN
 // CHECK: TBEGIN' comment without a subsequent 'NOLIN
 // CHECK: TEND' comment [clang-tidy-nolint]
 // CHECK: :[[@LINE-10]]:11: warning: single-argument constructors must be marked explicit
 // CHECK: :[[@LINE-10]]:12: warning: C-style casts are discouraged; use static_cast
 // CHECK: :[[@LINE-10]]:4: error: unmatched 'NOLIN
 // CHECK: TEND' comment without a previous 'NOLIN
 // CHECK: TBEGIN' comment [clang-tidy-nolint]
+
+// NOLINTBEGIN(google-explicit-constructor,google-readability-casting)
+class B { B(int i); };
+// NOLINTEND(google-explicit-constructor)
+auto Num2 = (unsigned int)(-1);
+// NOLINTEND(google-readability-casting)
+
+// Note: the expected output has been split over several lines so that clang-tidy
+//       does not see the "no lint" suppression comment and mistakenly assume it
+//       is meant for itself.
+// CHECK: :[[@LINE-9]]:4: error: unmatched 'NOLIN
+// CHECK: TBEGIN' comment without a subsequent 'NOLIN
+// CHECK: TEND' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-11]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-11]]:4: error: unmatched 'NOLIN
+// CHECK: TEND' comment without a previous 'NOLIN
+// CHECK: TBEGIN' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-13]]:13: warning: C-style casts are discouraged; use static_cast
+// CHECK: :[[@LINE-13]]:4: error: unmatched 'NOLIN
+// CHECK: TEND' comment without a previous 'NOLIN
+// CHECK: TBEGIN' comment [clang-tidy-nolint]

clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp

-// RUN: %check_clang_tidy %s google-explicit-constructor %t
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable
 
 class A { A(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTBEGIN
 class B1 { B1(int i); };
 // NOLINTEND
 

 // NOLINTEND
 class B5 { B5(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
 
 // NOLINTBEGIN(google-explicit-constructor)
 class C1 { C1(int i); };
 // NOLINTEND(google-explicit-constructor)
 
-// NOLINTBEGIN(*)
+// NOLINTBEGIN()
 class C2 { C2(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// NOLINTEND()
+
+// NOLINTBEGIN(*)
+class C3 { C3(int i); };
 // NOLINTEND(*)
 
 // NOLINTBEGIN(some-other-check)
-class C3 { C3(int i); };
+class C4 { C4(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
 // NOLINTEND(some-other-check)
 
 // NOLINTBEGIN(some-other-check, google-explicit-constructor)
-class C4 { C4(int i); };
-// NOLINTEND(some-other-check, google-explicit-constructor)
-
-// NOLINTBEGIN(some-other-check, google-explicit-constructor)
-// NOLINTEND(some-other-check)
 class C5 { C5(int i); };
-// NOLINTEND(google-explicit-constructor)
-
-// NOLINTBEGIN(some-other-check, google-explicit-constructor)
-// NOLINTEND(google-explicit-constructor)
-class C6 { C6(int i); };
-// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
-// NOLINTEND(some-other-check)
+// NOLINTEND(some-other-check, google-explicit-constructor)
 
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN(some-other-check)
-class C7 { C7(int i); };
+class C6 { C6(int i); };
 // NOLINTEND(some-other-check)
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN(some-other-check)
-class C8 { C8(int i); };
+class C7 { C7(int i); };
 // NOLINTEND(google-explicit-constructor)
 // NOLINTEND(some-other-check)
 
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN
-class C9 { C9(int i); };
+class C8 { C8(int i); };
 // NOLINTEND
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN
 // NOLINTBEGIN(google-explicit-constructor)
-class C10 { C10(int i); };
+class C9 { C9(int i); };
 // NOLINTEND(google-explicit-constructor)
 // NOLINTEND
 
 // NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
-class C11 { C11(int i); };
+class C10 { C10(int i); };
 // NOLINTEND(not-closed-bracket-is-treated-as-skip-all
 
 // NOLINTBEGIN without-brackets-skip-all, another-check
-class C12 { C12(int i); };
+class C11 { C11(int i); };
 // NOLINTEND without-brackets-skip-all, another-check
 
 #define MACRO(X) class X { X(int i); };
 
 MACRO(D1)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // CHECK-MESSAGES: :[[@LINE-4]]:28: note: expanded from macro 'MACRO
 

 
 #define MACRO_NO_LINT_INSIDE_MACRO \
   /* NOLINTBEGIN */                \
   class J { J(int i); };           \
   /* NOLINTEND */
 
 MACRO_NO_LINT_INSIDE_MACRO
 
-// CHECK-MESSAGES: Suppressed 18 warnings (18 NOLINT).
+// NOLINTBEGIN(google*)
+class C12 { C12(int i); };
+// NOLINTEND(google*)
+
+// NOLINTBEGIN(google*,-google*)
+class C13 { C13(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
+// NOLINTEND(google*,-google*)
+
+class C14 { C14(int x); void f() { int i; } };
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:40: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+
+// NOLINTBEGIN(*,-google*)
+class C15 { C15(int x); void f() { int i; } };
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
+// NOLINTEND(*,-google*)
+
+// CHECK-MESSAGES: Suppressed 19 warnings (19 NOLINT).

clang-tools-extra/test/clang-tidy/infrastructure/nolintnextline.cpp

 // NOLINTNEXTLINE
 class A { A(int i); };
 
 class B { B(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 class C { C(int i); };
 
 // NOLINTNEXTLINE(for-some-other-check)
 class D { D(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
-// NOLINTNEXTLINE(*)
+// NOLINTNEXTLINE()
 class D1 { D1(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
 
-// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+// NOLINTNEXTLINE(*)
 class D2 { D2(int i); };
 
-// NOLINTNEXTLINE(google-explicit-constructor)
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
 class D3 { D3(int i); };
 
-// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+// NOLINTNEXTLINE(google-explicit-constructor)
 class D4 { D4(int i); };
 
-// NOLINTNEXTLINE without-brackets-skip-all, another-check
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
 class D5 { D5(int i); };
 
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class D6 { D6(int i); };
+
 // NOLINTNEXTLINE
 
 class E { E(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 // NOLINTNEXTLINE
 //
 class F { F(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 #define MACRO(X) class X { X(int i); };
 MACRO(G)
 // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
 // NOLINTNEXTLINE
 MACRO(H)
 
 #define MACRO_NOARG class I { I(int i); };
 // NOLINTNEXTLINE
 MACRO_NOARG
 
-// CHECK-MESSAGES: Suppressed 9 warnings (9 NOLINT)
+// NOLINTNEXTLINE(google*)
+class I1 { I1(int i); };
+
+// NOLINTNEXTLINE(google*,-google*)
+class I2 { I2(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+class I3 { I3(int x); void f() { int i; } };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+// CHECK-MESSAGES: :[[@LINE-2]]:38: warning: unused variable 'i' [clang-diagnostic-unused-variable]
+
+// NOLINTNEXTLINE(*,-google*)
+class I4 { I4(int x); void f() { int i; } };
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// CHECK-MESSAGES: Suppressed 11 warnings (11 NOLINT)
 
-// RUN: %check_clang_tidy %s google-explicit-constructor %t --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable