[clang-tidy] Performance improvements for NOLINTBEGIN/END blocks

[salman-javed-nz]

Summary

Summary

Support for NOLINT(BEGIN/END) blocks (implemented in D108560) is currently very costly. This patch aims to improve the performance with the following changes:

• The use of tokenized NOLINTs instead of a bunch of repetitive ad-hoc string operations (find(), split(), slice(), regex matching etc).
• The caching of NOLINT(BEGIN/END) block locations. Determining these locations each time a new diagnostic is raised is wasteful as it requires reading and parsing the entire source file.

Move NOLINT-specific code from ClangTidyDiagnosticConsumer to new purpose-built class NoLintDirectiveHandler and simplify public interface.

Use tokenized NOLINTs

Currently, the implementation repeats a lot of string operations, such as:

• searching for magic strings like "NOLINT", "NOLINTNEXTLINE", "NOLINTBEGIN", "NOLINTEND"
• searching for and splitting on \n and , characters
• searching for the check list enclosed in (brackets)
• constructing temporary GlobList objects
• regex operations

The implementation is rather "stringly typed".

This patch improves things by introducing a new NoLintToken class (implementation detail in NoLintDirectiveHandler.cpp). This class stores parsed information about a NOLINT marker, such as its type (NOLINT/NEXTLINE/BEGIN/END), its location in the source file, and whether it specifies any checks. The NOLINT handler has been adopted to use this class. Passing around NoLintTokens is preferable to passing raw string content, because NoLintTokens are already parsed and are smaller in size.

Cache the locations of NOLINT(BEGIN/END) blocks.

The NOLINT handler runs each time a diagnostic is raised. It has to read the entire source file to determine the locations of the NOLINT(BEGIN/END) blocks. If the diagnostic is due to a problem in an included header, the handler has to read the included header in its entirety as well. Currently, the implementation throws away this information and has to regenerate it the next time there is a diagnostic.

This patch adds a cache for this information. It is not enough to keep a cache for just the current file being analyzed. If it is to be performant, the cache should contain data for all files that have been analyzed, not just the current one. We do not want to invalidate the cache as we context switch between files as we trace diagnostics generated in macros and headers.

The performance should be better now (see test results below). In any case, to ensure we aren't paying for something that we don't need, we only generate the NOLINT(BEGIN/END) cache if we didn't find a valid NOLINT or NOLINTNEXTLINE first (checking for these is usually cheaper so they are checked first).

Performance testing

YMMV with the performance improvements you see. It depends on the code you are analyzing - factors include the file size, number of included files, number of diagnostics, number of NOLINTs...
To give some indication of the performance improvement on a real world project, I have run clang-tidy over the LLVM codebase and got the following results:

time clang-tools-extra\clang-tidy\tool\run-clang-tidy.py -p build -checks="-*,bugprone-*,cppcoreguidelines-*,llvm-*,modernize-*,misc-*,performance-*,readability-*" llvm

Baseline (NOLINT, NOLINTNEXTLINE support only):
3h 44m 05s

Current NOLINTBEGIN/END implementation (BEFORE performance improvements):
7h 36m 37s

New NOLINTBEGIN/END implementation (AFTER performance improvements):
3h 40m 05s

So the new implementation is about the same speed as the baseline (before NOLINTBEGIN/END) implementation. The performance hit added by the NOLINTBEGIN/END support seems to have been counter-acted by the general performance improvements to do with NoLintToken etc.

Other changes

The NOLINT handler code has come to a stage that it deserves its own CPP file instead of hitching a ride on ClangTidyDiagnosticConsumer.cpp, which is already pretty long. As suggested by review comments, I have used a PIMPL for the NOLINT handler class.

I have split up/renamed some unit test files so that they test one aspect per file, and added new tests to lock in some long-standing behavior.

[salman-javed-nz]

Remove unnecessary llvm:: qualification.

[salman-javed-nz]

Updated the review's edit permissions. Sorry about that, @kadircet.

[kadircet]

some drive-by comments, i'll be AFK for a while though. so please don't block this on my approval if people disagree.

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

197

this change looks unrelated to the current patch.

455

as mentioned above, if we were to make this NoLintPragmaHandler::isLineWithinNoLint in which NoLintPragmaHandler owns the cache of NoLintTokens per FileID. We can just first populate the cache (or use the already cached tokens) and then iterate over them here to see if any of them suppresses diagnostic at hand.

I don't think it's worth optimizing the iteration over tokens, as we won't have more than a handful of nolint tokens in a single file. but if that assumption turned out to be wrong, i suppose we can also do a binary search over tokens rather than iterating over all of them in the future.

546

nit: there's also llvm::StringSwitch

563

this creates new regexes at each call, what about building the glob once in constructor and storing that instead of Checks ?

580

performing this check once during construction sounds better than performing it for every diagnostics.

602

looks like an unrelated change, maybe move to a separate patch?

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

66

instead of having an unknown type here and making it a concern for all the places handling the token, can we just ignore such tokens during generation?

it'd probably imply having a constructor for the object which takes Location, Type and Checks as input.

76

similar to above is NOLINT() useful at all? maybe we should just not generate such tokens?

94

why not just store a SourceLocation for the end and make sure we only construct valid ones?

236

what about a narrower interface? e.g. moving shouldSuppressDiagnostics into ClangTidyContext. It already takes the Context as a mutable-ref.

265

what about just having:

class NoLintPragmaHandler;
std::unique_ptr<NoLintPragmaHandler> NoLintHandler;

And making all of this an implementation detail (including the NoLintToken/Block structs above)?

We can then have a NoLintPragmaHandler::isLineWithinNoLint which performs all the checks while filling the cache if need be?

[salman-javed-nz]

Thanks for taking a look. I will update the diff to address your comments. Have a good new years break.

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

197

I suppose it is. It's fixing a lint issue in a function used by the NOLINTBEGIN functionality, but not needed to implement the caching functionality. I don't have strong feelings about whether this change is bundled in this patch or not.

602

This is needed for this patch. The diagnostic in question may be suppressed (shouldSuppressDiagnostic() = true) but the cache generation could return errors to do with badly formed NOLINTBEGIN blocks for other checks in the file.

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

76

NOLINT() is a pretty silly statement, but it is specially mentioned in the clang-tidy documentation.
I didn't want the parser to just swallow NOLINT()s as if they were never there, because I still want to pick up errors like:

// NOLINTBEGIN()
// NOLINTEND(some-check)
   ^
   Error: NOLINTEND does not match previous NOLINTBEGIN

[salman-javed-nz]

Updated according to review comments

• Move NoLintToken and other support classes to the CPP file.
• Create a NoLintPragmaHandler class which owns the cache. Access this class through a PIMPL.
• Use StringSwitch instead of StringMap.
• Store the GlobList object instead of repeatedly constructing temporaries.
• Remove Unknown from the NoLintType enum.
• Add constructors to NoLintToken and NoLintBlockToken that can ensure that the objects are initialized with valid values.
• Move shouldSuppressDiagnostic() into ClangTidyContext. This function took ClangTidyContext as a mutable ref anyway.

[salman-javed-nz]

Move comments

[salman-javed-nz]

Friendly ping.

Would be good to get these performance improvements into trunk soon, so that we're not prolonging the time that people are putting up with the current slow implementation. Also, if I understand correctly, it will be time for a LLVM 14.0.0 rc branch soon, and all going well, the NOLINTBEGIN feature would be debuting in it. It would be nice if the feature shipped in this faster incarnation. It would be a pity if users forgo using this feature because they are put off by the slowness of it.

@kadircet as the clangd expert, I'm especially interested in your thoughts on this patch, as clangd is sensitive to clang-tidy's performance. I compiled clangd and tested it with the vscode-clangd extension. Everything worked as expected when moving lines around, applying fixits, and switching tabs. Performance looked OK to me too, but I haven't used clangd all that much so I don't have a good baseline on what is normal performance.

Thanks a lot.

[carlosgalvezp]

Amazing job @salman-javed-nz ! Here's some initial comments. Reviewing the NoLintPragmaHandler.cpp will take some more time from me. It would have been easier to see the diff if the contents had been moved as an NFC patch to a separate file, and then applying the optimizations in this patch. But it's done now so I'll deal with it :)

clang-tools-extra/clang-tidy/CMakeLists.txt

20 ↗

(On Diff #398087)

I think "Pragma" is a very specific term, used for example in #pragma gcc diagnostic or // IWYU pragma: keep, but in clang-tidy we don't use the word pragma, so that might be confusing. What about renaming it to NoLintHandler.cpp or NoLintDirectiveHandler.cpp?

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

162

Why not SmallVector? Sounds like the Impl is some "private" implementation?

clang-tools-extra/clang-tidy/NoLintPragmaHandler.h

18 ↗

(On Diff #398087)

Why not SmallVector?

[salman-javed-nz]

In D116085#3257210, @carlosgalvezp wrote:

Amazing job @salman-javed-nz ! Here's some initial comments. Reviewing the NoLintPragmaHandler.cpp will take some more time from me. It would have been easier to see the diff if the contents had been moved as an NFC patch to a separate file, and then applying the optimizations in this patch. But it's done now so I'll deal with it :)

Thank you :)

You might find the previous revision of this patch useful then (https://reviews.llvm.org/D116085?id=395606). In this earlier revision I made the change in the original file ClangTidyDiagnosticHandler.cpp.
There's been some refinements to that code in the subsequent revisions (the subsequent revisions aren't merely moving the changes to a separate file) but it's still useful as a way to get the general gist of the new NOLINT tokenization and location caching process.

clang-tools-extra/clang-tidy/CMakeLists.txt

20 ↗

(On Diff #398087)

Your suggestions sound better. I'm not attached to the name NoLintPragmaHandler.

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

162

SmallVectorImpl is the recommended type to use for output params.

• SmallVector<T> is actually SmallVectorImpl<T, N = some_default_size> under the hood.
• By declaring the function param as SmallVectorImpl I'm saying that I don't care about the size of the vector that the caller passes in.

See https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h

Even though it has “Impl” in the name, SmallVectorImpl is widely used and is no longer “private to the implementation”.

clang-tools-extra/clang-tidy/NoLintPragmaHandler.h

18 ↗

(On Diff #398087)

See my other comment about SmallVectorImpl.

[salman-javed-nz]

Update according to review (rename NoLintPragmaHandler class to NoLintDirectiveHandler)

[kadircet]

here are some more comments, can't say I've been able to go through all of the patch yet, unfortunately it's considerably big in size. it would be great to get rid of some of those extra code by just dropping accessors/classes when not needed.

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

160

i think having an AllowIO parameter is quite confusing here. It doesn't, and probably won't, control all the IO that might be done directly or transitively through ClangTidyContext as one would naturally expect.

what about just making it a parameter to shouldSuppressDiagnostic call ?
when IO is allowed, the suppression logic will be able to fetch the contents and work as expected so no problems there.
when it is not, we'll either fail to get the buffer and bail out early, or we'll get the buffer from memory and again everything will work as intended.
In either case the cache is never poisoned, so we can call shouldSuppressDiagnostic with different values of AllowIO on a single instance of ClangTidyContext.
The same applies to EnableNoLintBlocks too. So lets move both back into the parameters of shouldSuppressDiagnostics.

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

123

why are we moving these two parameters here?

144

can we not introduce a new error type in this patch, as it is already doing a good amount of changes?

288

no need to make this dependency weak, let's just depend on the public header and make this NoLintPragmaHandler NoLintHandler.

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

37 ↗

(On Diff #398087)

place class/struct definitions in this file inside an anonymous namespace.

63 ↗

(On Diff #398087)

why not just a basic struct ? i don't think all of these accessors/constructors carry their weight (as mentioned below once the ChecksGlob is not lazily computed, there should be no need for any of these).

63 ↗

(On Diff #398087)

can we please inline the definitions (if there's a good reason to stick with the class)?

90 ↗

(On Diff #398087)

this deserves a comment about the difference of None vs empty

97 ↗

(On Diff #398087)

s/equivalent is/equivalent to/

133 ↗

(On Diff #398087)

i think it's fine to assume that this check will always be performed (otherwise why would user have the nolint comment in their code in the first place). so i don't think all the complexity around making this mutable and lazily computing is worth it.

213 ↗

(On Diff #398087)

Tokenize the contents in \p Buffer between \p BeginPos and \p EndPos. Not necessarily the entire buffer, right?

214 ↗

(On Diff #398087)

all of this seems a little extra, why not something like:

vector<NoLinttoken> getNoLintTokens(StringRef Buffer, size_t StartOffset, size_t EndOffset) {
vector<NoLinttoken> Results;
Buffer = Buffer.slice(StartOffset, EndOffset);
while(auto Token = getNextNoLintToken(Buffer)) { // Also trims Buffer, returns None when not found.
  Token.pos += StartOffset;
  Results.push_back(std::move(Token));
}
return Results;
}

350 ↗

(On Diff #398087)

i think it's better to propagate NoLintErrors all the way until error generation. as it looks quite confusing as the class state.

407 ↗

(On Diff #398087)

s/exhaustive/expensive

420 ↗

(On Diff #398087)

filenames in source manager could be misleading depending on how the file is accessed (there might be an override, symlinks, includemaps etc.) and different fileids can also refer to same file (e.g. when header is not include guarded). so both can result in reading the same file contents multiple times, but at least fileids are not strings so should be better keys for the cache && get rid of this step around fetching the filename from fileid.

Hence can we switch the cache from stringmap to a densemap<fileid, vector<nolinttoken>> instead.

469 ↗

(On Diff #398087)

maybe just 'NOLINTBEGIN' comment without a matching 'NOLINTEND' comment, vice versa for the other case.

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp

1 ↗

(On Diff #398087)

no run lines or anything here (and the following file)

1 ↗

(On Diff #398087)

oops, nevermind this one. I've seen the testing entry down below but forgot to delete the comment.

[carlosgalvezp]

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

347

Nit: tab with spaces

390

As a non-native English speaker, I appreciate the name change :)

422

Nit: tab with spaces

448

Don't we usually use SourceLocation objects for this?

480

Tab with spaces

563

Maybe good to keep the original comment about why negative globs are excluded?

563

+1. Also, in case it helps, you can use CachedGlobList.

[salman-javed-nz]

Thank you very much to both of you for having a look at the patch. Yes, I agree it is a large patch, and I could have done a better job splitting it up into more manageable chunks.

One reason this change is so big is because I set myself an ambitious target for the performance gains I wanted to achieve in this patch. After spending more time with the current implementation in master/main, I began to realize how expensive it really is. Others have too, as demonstrated by the fact that multiple commits have been made to disable the functionality in downstream projects. So wherever I saw a more efficient way of doing things, I didn't want to rule out making a change, even if it means parts of the code had to be refactored or rewritten.

There is a balance I have to strike between achieving my primary objective and minimizing the impact to the code base, and I admit I haven't got it right so far. We can discuss where exactly the balance should be, but let me tidy up the more egregious instances of unnecessary code first, like the public accessors that don't hold their weight that you mentioned.

To make your lives easier, I will address your requested changes as separate updates to this patch.

[salman-javed-nz]

Move AllowIO and EnableNoLintBlocks from ClangTidyContext() back to shouldSuppressDiagnostic().

[salman-javed-nz]

Store the NoLintDirectiveHandler object directly in ClangTidyContext, instead of doing it through a unique_ptr.

[salman-javed-nz]

Inline short methods into classes

[salman-javed-nz]

Put implementation-related classes in an anonymous namespace

[salman-javed-nz]

Fix errors in comments

[salman-javed-nz]

NoLintToken:

• remove public getter methods for Type and Pos - just use the fields directly
• consolidate the constructors into one constructor
• add comment about the difference about between Checks = None and Checks = ""
• add back the comment about why negative globs are ignored
• don't lazy compute the glob

[salman-javed-nz]

Instead of defining a new error type for the NoLintDirectiveHandler, use the pre-existing error type clang::tooling::Diagnostic

[salman-javed-nz]

Replace NoLintTokenizer class with a single function, getNoLints()

[salman-javed-nz]

Pass the NoLintErrors SmallVector all the way through the call stack, instead of storing it in the class

[salman-javed-nz]

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

123

Parameters moved back to shouldSuppressDiagnostic().

[salman-javed-nz]

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

63 ↗

(On Diff #398087)

I've removed as many accessors and constructors as I can from this class. I believe there's still some value in hiding the Optional<std::string> Checks and CachedGlobList ChecksGlob behind methods, because if you modify one, you must modify the other for the NoLintToken object to make sense.

[salman-javed-nz]

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

448

Don't we usually use SourceLocation objects for this?

Considering that we're working with a string buffer of the file contents, I think using buffer indices of type size_t is appropriate.

[salman-javed-nz]

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

603

This is NOT a tab character, even though it looks like one on Phabricator.

[salman-javed-nz]

Every review comment so far should be addressed now, with the exception of the 2 points below.

I've updated the diff a handful of times showing each stage of my changes. Thought this may be more reviewer friendly.

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

420 ↗

(On Diff #398087)

filenames in source manager could be misleading depending on how the file is accessed (there might be an override, symlinks, includemaps etc.) and different fileids can also refer to same file (e.g. when header is not include guarded). so both can result in reading the same file contents multiple times, but at least fileids are not strings so should be better keys for the cache && get rid of this step around fetching the filename from fileid.

Hence can we switch the cache from stringmap to a densemap<fileid, vector<nolinttoken>> instead.

I used FileIDs in an earlier attempt at this patch, but I had issues when specifying multiple input files to clang-tidy on the command line, e.g. clang-tidy file1.cpp file2.cpp. The analysis of each file begins with a fresh instance of SourceManager, so both file1.cpp and file2.cpp have a FileID of 1. It looks to me that I would need to clear the NoLintBlock cache each time a new SourceManager is used.

This is what the nolintbeginend-multiple-TUs.cpp test is all about.

The file path is a SourceManager-independent means of identifying the file for use in a map, so that's why it's being used. What are your thoughts on what map key to use? Neither looks ideal to me.

469 ↗

(On Diff #398087)

maybe just 'NOLINTBEGIN' comment without a matching 'NOLINTEND' comment, vice versa for the other case.

Could we do this in another patch? Quite a number of unit tests will need updating.

[kadircet]

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

213

we don't seem to be passing allowio/enablenolintblocks ?

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

89 ↗

(On Diff #402308)

nit: Any reason for not making this class move-only ? everytime we copy, we are creating globs from scratch and losing the cache. i think it should be fine if we just did emplace_back in all places and moved tokens around rather than taking copies/pointers.

133 ↗

(On Diff #402308)

nit: static constexpr llvm::StringLiteral

141 ↗

(On Diff #402308)

nit: break instead?

156 ↗

(On Diff #402308)

nit: maybe drop the lambda? direct version has similar/less indentation.

llvm::Optional<std::string> Checks;
if(Pos < Buffer.size() && Buffer[Pos] == '(') {
  auto ClosingBracket = Buffer.find_first_of("\n)", ++Pos);
  if (ClosingBracket != Buffer.npos && Buffer[ClosingBracket] == ')')
   Checks = Buffer.slice(Pos, ClosingBracket).str();
}

170 ↗

(On Diff #402308)

nit: you can drop the NoLintToken

427 ↗

(On Diff #402308)

no need for class here.

clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h

41 ↗

(On Diff #402308)

i feel like this is still an implementation detail of the ClangTidyContext, we have it publicly available just to reduce amount of extra code inside a single source file. so i think it makes sense for these parameters to not have defaults (as the one in ClangTidyContext already has).

[salman-javed-nz]

ClangTidyContext::shouldSuppressDiagnostic():

• Hook up AllowIO and EnableNoLintBlocks to NoLintDirectiveHandler::shouldSuppress()

NoLintToken:

• Remove copy ctor and assignment operator. Class is now move-only.

getNoLints():

• Use llvm::StringLiteral for string literal
• Break out of the loop when there are no more NOLINT strings
• Determine NOLINT checks string inline instead of in a lambda
• Remove the NoLintToken() in the call to emplace_back()

NoLintDirectiveHandler:

• std::make_unique<class Impl> --> std::make_unique<Impl>
• Remove default values for AllowIO and EnableNoLintBlocks parameters

[kadircet]

thanks, lgtm!

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

187 ↗

(On Diff #402774)

let's drop the & here to make sure caller does an explicit std::move. since the helper is actually consuming the tokens inside the vector and in theory it shouldn't be used afterwards.

[salman-javed-nz]

Review comment:
formNoLintBlocks() - drop the & so the vector must be std::moved in

[carlosgalvezp]

Looks great!

[nemanjai]

This is causing buildbot failures with -Werror (i.e. https://lab.llvm.org/buildbot/#/builders/57/builds/14322/steps/5/logs/stdio). Please fix or revert. The failure is:

.../llvm-project/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:204:38: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
        CompletedBlocks.emplace_back(std::move(Stack.pop_back_val()), NoLint);

[nemanjai]

In D116085#3272200, @nemanjai wrote:

This is causing buildbot failures with -Werror (i.e. https://lab.llvm.org/buildbot/#/builders/57/builds/14322/steps/5/logs/stdio). Please fix or revert. The failure is:

.../llvm-project/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp:204:38: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
        CompletedBlocks.emplace_back(std::move(Stack.pop_back_val()), NoLint);

Sorry, I didn't realize you already reverted as there was no mention in the revert of the original commit.

[salman-javed-nz]

In D116085#3272200, @nemanjai wrote:

This is causing buildbot failures with -Werror (i.e. https://lab.llvm.org/buildbot/#/builders/57/builds/14322/steps/5/logs/stdio). Please fix or revert.

Thanks for the heads up. I have reverted in 8e29d19b8d29db1ad60af3a8921aad7bbfc24435 while I investigate.

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

struct ClangTidyStats {
   unsigned ErrorsIgnoredLineFilter = 0;
 
   unsigned errorsIgnored() const {
     return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
            ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }
 };
 
+/// Records the presence of a NOLINT comment - its type, location, checks -
+/// as parsed from the file's character contents.
+struct NoLintToken {
+  enum class Type { Unknown, NoLint, NoLintNextLine, NoLintBegin, NoLintEnd };
+  Type NoLintType = Type::Unknown;
+
+  /// The location of the first character, "N", in "NOLINT".
+  SourceLocation Loc;
+
+  /// Whether this NOLINT comment provides an optional list of checks in
+  /// brackets, i.e. "NOLINT(check1, check2, etc)".
+  bool SpecifiesChecks = false;
+  // NOTE: at first glance, *it may seem* that this bool is redundant when the
+  // `Checks.empty()` method already exists.
+  // However, `Checks.empty()` cannot differentiate "NOLINT" from "NOLINT()".
+  // "NOLINT" should suppress all checks, while "NOLINT()" should suppress zero
+  // checks. In both cases, `Checks.empty()` returns true.
+
+  /// The optional list of checks.
+  StringRef Checks;
+
+  /// Convert a string like "NOLINTNEXTLINE" to its enum `Type::NoLintNextLine`.
+  static Type strToType(StringRef S);
+
+  /// Whether this NOLINT comment applies to the provided `CheckName`.
+  bool suppresses(StringRef CheckName) const;
+};
+
+/// Represents a source range within a pair of NOLINT(BEGIN/END) comments.
+struct NoLintBlockToken {
+  NoLintToken Begin;
+  NoLintToken End;
+
+  /// Whether the two tokens can be paired together to form a block.
+  static bool isValidPair(const NoLintToken &Begin, const NoLintToken &End,
+                          const SourceManager &SM);
+
+  /// Whether the provided `Loc` lies between the NOLINT(BEGIN/END) comments.
+  bool contains(SourceLocation Loc, const SourceManager &SM) const;
+
+  /// Whether the provided diagnostic is within and is suppressible by this
+  /// block of NOLINT(BEGIN/END) comments.
+  bool suppresses(SourceLocation DiagLoc, StringRef DiagName,
+                  const SourceManager &SM) const;
+};
+
 /// Every \c ClangTidyCheck reports errors through a \c DiagnosticsEngine
 /// provided by this context.
 ///
 /// A \c ClangTidyCheck always has access to the active context to report
 /// warnings like:
 /// \code
 /// Context->Diag(Loc, "Single-argument constructors must be explicit")
 ///     << FixItHint::CreateInsertion(Loc, "explicit ");
 /// \endcode
 class ClangTidyContext {
 public:
   /// Initializes \c ClangTidyContext instance.
   ClangTidyContext(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
                    bool AllowEnablingAnalyzerAlphaCheckers = false);
   /// Sets the DiagnosticsEngine that diag() will emit diagnostics to.
   // FIXME: this is required initialization, and should be a constructor param.
   // Fix the context -> diag engine -> consumer -> context initialization cycle.
   void setDiagnosticsEngine(DiagnosticsEngine *DiagEngine) {
     this->DiagEngine = DiagEngine;
   }
 
   ~ClangTidyContext();
 
   /// Report any errors detected using this method.
   ///
   /// This is still under heavy development and will likely change towards using
   /// tablegen'd diagnostic IDs.
   /// FIXME: Figure out a way to manage ID spaces.
   DiagnosticBuilder diag(StringRef CheckName, SourceLocation Loc,
                          StringRef Message,
                          DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
   DiagnosticBuilder diag(StringRef CheckName, StringRef Message,
                          DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
   DiagnosticBuilder diag(const ClangTidyError &Error);
 
   /// Report any errors to do with reading the configuration using this method.
   DiagnosticBuilder
   configurationDiag(StringRef Message,
                     DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
   /// Sets the \c SourceManager of the used \c DiagnosticsEngine.
   ///
   /// This is called from the \c ClangTidyCheck base class.
   void setSourceManager(SourceManager *SourceMgr);
 
   /// Should be called when starting to process new translation unit.
   void setCurrentFile(StringRef File);
 
   /// Returns the main file name of the current translation unit.
   StringRef getCurrentFile() const { return CurrentFile; }
 
   /// Sets ASTContext for the current translation unit.
   void setASTContext(ASTContext *Context);
 
   /// Gets the language options from the AST context.
   const LangOptions &getLangOpts() const { return LangOpts; }
 
   /// Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
   std::string getCheckName(unsigned DiagnosticID) const;
DiagLevelAndFormatString getDiagLevelAndFormatString(unsigned DiagnosticID,
                                                        SourceLocation Loc) {
     return DiagLevelAndFormatString(
         static_cast<DiagnosticIDs::Level>(
             DiagEngine->getDiagnosticLevel(DiagnosticID, Loc)),
         std::string(
             DiagEngine->getDiagnosticIDs()->getDescription(DiagnosticID)));
   }
 
+  /// Get a list of all NOLINT(BEGIN/END) blocks in the file.
+  /// NOTE: this is a heavy operation, therefore this method returns a cached
+  /// result on subsequent calls.
+  const SmallVector<NoLintBlockToken> &
+  getNoLintBlocks(FileID File, SmallVectorImpl<ClangTidyError> &Errors,
+                  bool AllowIO = true) const;
+
 private:
   // Writes to Stats.
   friend class ClangTidyDiagnosticConsumer;
 
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;
private:
   std::string CurrentBuildDirectory;
 
   llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
 
   bool Profile;
   std::string ProfilePrefix;
 
   bool AllowEnablingAnalyzerAlphaCheckers;
+
+  mutable llvm::DenseMap<FileID, SmallVector<NoLintBlockToken>> NoLintCache;
 };
 
 /// Check whether a given diagnostic should be suppressed due to the presence
 /// of a "NOLINT" suppression comment.
 /// This is exposed so that other tools that present clang-tidy diagnostics
 /// (such as clangd) can respect the same suppression rules as clang-tidy.
 /// This does not handle suppression of notes following a suppressed diagnostic;
 /// that is left to the caller as it requires maintaining state in between calls
 /// to this function.
 /// If `AllowIO` is false, the function does not attempt to read source files
 /// from disk which are not already mapped into memory; such files are treated
 /// as not containing a suppression comment.
 /// \param EnableNolintBlocks controls whether to honor NOLINTBEGIN/NOLINTEND
 /// blocks; if false, only considers line-level disabling.
 /// If suppression is not possible due to improper use of "NOLINT" comments -
 /// for example, the use of a "NOLINTBEGIN" comment that is not followed by a
 /// "NOLINTEND" comment - a diagnostic regarding the improper use is returned
 /// via the output argument `SuppressionErrors`.
 bool shouldSuppressDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info,
     ClangTidyContext &Context,
     SmallVectorImpl<ClangTidyError> &SuppressionErrors, bool AllowIO = true,
     bool EnableNolintBlocks = true);
 
 /// Gets the Fix attached to \p Diagnostic.
 /// If there isn't a Fix attached to the diagnostic and \p AnyFix is true, Check
 /// to see if exactly one note has a Fix and return it. Otherwise return
 /// nullptr.
 const llvm::StringMap<tooling::Replacements> *
 getFixIt(const tooling::Diagnostic &Diagnostic, bool AnyFix);
 

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

 ClangTidyError::ClangTidyError(StringRef CheckName,
                                ClangTidyError::Level DiagLevel,
                                StringRef BuildDirectory, bool IsWarningAsError)
     : tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory),
       IsWarningAsError(IsWarningAsError) {}
 
 ClangTidyContext::ClangTidyContext(
     std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
     bool AllowEnablingAnalyzerAlphaCheckers)
     : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
       Profile(false),
       AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
   // Before the first translation unit we can get errors related to command-line
   // parsing, use empty string for the file name in this case.
   setCurrentFile("");
 }
 

 }
 
 DiagnosticBuilder ClangTidyContext::diag(const ClangTidyError &Error) {
   SourceManager &SM = DiagEngine->getSourceManager();
   llvm::ErrorOr<const FileEntry *> File =
       SM.getFileManager().getFile(Error.Message.FilePath);
   FileID ID = SM.getOrCreateFileID(*File, SrcMgr::C_User);
   SourceLocation FileStartLoc = SM.getLocForStartOfFile(ID);
-  SourceLocation Loc = FileStartLoc.getLocWithOffset(Error.Message.FileOffset);
+  SourceLocation Loc = FileStartLoc.getLocWithOffset(
+      static_cast<SourceLocation::IntTy>(Error.Message.FileOffset));
   return diag(Error.DiagnosticName, Loc, Error.Message.Message,
               static_cast<DiagnosticIDs::Level>(Error.DiagLevel));
 }
 
 DiagnosticBuilder ClangTidyContext::configurationDiag(
     StringRef Message,
     DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
   return diag("clang-tidy-config", Message, Level);
 }
 
 void ClangTidyContext::setSourceManager(SourceManager *SourceMgr) {
   DiagEngine->setSourceManager(SourceMgr);
 }
 
 void ClangTidyContext::setCurrentFile(StringRef File) {
   CurrentFile = std::string(File);
   CurrentOptions = getOptionsForFile(CurrentFile);
   CheckFilter = std::make_unique<CachedGlobList>(*getOptions().Checks);
   WarningAsErrorFilter =
       std::make_unique<CachedGlobList>(*getOptions().WarningsAsErrors);
 }
 
 void ClangTidyContext::setASTContext(ASTContext *Context) {
   DiagEngine->SetArgToStringFn(&FormatASTNodeDiagnosticArgument, Context);
if (!Errors.empty()) {
     } else {
       ++Context.Stats.ErrorsDisplayed;
     }
   }
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
 
-static bool isNOLINTFound(StringRef NolintDirectiveText, StringRef CheckName,
-                          StringRef Line, size_t *FoundNolintIndex = nullptr,
-                          StringRef *FoundNolintChecksStr = nullptr) {
-  if (FoundNolintIndex)
-    *FoundNolintIndex = StringRef::npos;
-  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);
-      if (FoundNolintChecksStr)
-        *FoundNolintChecksStr = ChecksStr;
-      // Allow specifying a few checks with a glob expression, ignoring
-      // negative globs (which would effectively disable the suppression).
-      GlobList Globs(ChecksStr, /*KeepNegativeGlobs=*/false);
-      if (!Globs.contains(CheckName))
-        return false;
+/// Return a list of all NOLINT comments in the provided `Buffer`.
+/// `Buffer` can be a line or the entire file contents.
+static SmallVector<NoLintToken> findNoLints(StringRef Buffer,
+                                            SourceLocation BufferLoc) {
+  static constexpr StringRef NoLintStr = "NOLINT";
+  SmallVector<NoLintToken> NoLints;
+  size_t Pos = 0;
+
+  // Keep checking for NOLINTs until all of them are found, or the buffer is
+  // exhausted.
+  while (Pos < Buffer.size()) {
+    // Look for the next "NOLINT".
+    const size_t NoLintPos = Buffer.find(NoLintStr, Pos);
+    if (NoLintPos == StringRef::npos)
+      break;
+    NoLintToken NoLint;
+    NoLint.Loc = BufferLoc.getLocWithOffset(
+        static_cast<SourceLocation::IntTy>(NoLintPos));
+
+    // Look for characters immediately after the "NOLINT" (e.g. the "NEXTLINE"
+    // in "NOLINTNEXTLINE").
+    Pos = NoLintPos + NoLintStr.size();
+    while (Pos < Buffer.size() && isalpha(Buffer[Pos]) != 0)
+      ++Pos;
+    const StringRef NoLintType = Buffer.slice(NoLintPos, Pos);
+    NoLint.NoLintType = NoLintToken::strToType(NoLintType);
+    if (NoLint.NoLintType == NoLintToken::Type::Unknown)
+      continue;
+
+    // Look for "(checks-list)" immediately after.
+    if (Pos < Buffer.size() && Buffer[Pos] == '(') {
+      const size_t ChecksPos = ++Pos;
+      // Look for closing bracket (or end of the line - whichever comes first).
+      while (Pos < Buffer.size() && Buffer[Pos] != ')' && Buffer[Pos] != '\n')
+        ++Pos;
+      if (Pos < Buffer.size() && Buffer[Pos] == ')') {
+        NoLint.Checks = Buffer.slice(ChecksPos, Pos);
+        NoLint.SpecifiesChecks = true;
       }
     }
 
-  if (FoundNolintIndex)
-    *FoundNolintIndex = NolintIndex;
-
-  return true;
+    NoLints.emplace_back(NoLint);
+  }
+
+  return NoLints;
+}
+
+static bool isNoLintFound(StringRef Line, NoLintToken::Type NoLintType,
+                          StringRef CheckName) {
+  SmallVector<NoLintToken> NoLints = findNoLints(Line, SourceLocation());
+  return std::any_of(
+      NoLints.begin(), NoLints.end(), [&](const NoLintToken &NoLint) {
+        return NoLint.NoLintType == NoLintType && NoLint.suppresses(CheckName);
+      });
 }
 
 static llvm::Optional<StringRef> getBuffer(const SourceManager &SM, FileID File,
                                            bool AllowIO) {
   return AllowIO ? SM.getBufferDataOrNone(File)
                  : SM.getBufferDataIfLoaded(File);
 }
 
 static ClangTidyError createNolintError(const ClangTidyContext &Context,
                                         const SourceManager &SM,
-                                        SourceLocation Loc,
-                                        bool IsNolintBegin) {
+                                        const NoLintToken &NoLint) {
   ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
                        Context.getCurrentBuildDirectory(), false);
   StringRef Message =
-      IsNolintBegin
+      NoLint.NoLintType == NoLintToken::Type::NoLintBegin
           ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
              "END' comment")
           : ("unmatched 'NOLINTEND' comment without a previous 'NOLINT"
              "BEGIN' comment");
-  Error.Message = tooling::DiagnosticMessage(Message, SM, Loc);
+  Error.Message = tooling::DiagnosticMessage(Message, SM, NoLint.Loc);
   return Error;
 }
 
-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;
-  StringRef NolintChecksStr;
-  for (const auto &Line : Lines) {
-    if (isNOLINTFound("NOLINTBEGIN", CheckName, Line, &NolintIndex,
-                      &NolintChecksStr)) {
-      // Check if a new block is being started.
-      NolintBegins.emplace_back(std::make_pair(
-          LinesLoc.getLocWithOffset(NolintIndex), NolintChecksStr));
-    } else if (isNOLINTFound("NOLINTEND", CheckName, Line, &NolintIndex,
-                             &NolintChecksStr)) {
-      // Check if the previous block is being closed.
-      if (!NolintBegins.empty() &&
-          NolintBegins.back().second == NolintChecksStr) {
-        NolintBegins.pop_back();
-      } else {
-        // Trying to close a nonexistent block. Return a diagnostic about this
+/// Look through the entire `Buffer` and keep track of all NOLINT(BEGIN/END)
+/// blocks. If any invalid blocks are found, e.g. a `BEGIN` comment without a
+/// corresponding `END`, return a diagnostic about it in `Errors`.
+static SmallVector<NoLintBlockToken>
+collectNoLintBlocks(const ClangTidyContext &Context, const SourceManager &SM,
+                    StringRef Buffer, SourceLocation BufferLoc,
+                    SmallVectorImpl<ClangTidyError> &Errors) {
+  const SmallVector<NoLintToken> NoLints = findNoLints(Buffer, BufferLoc);
+  SmallVector<const NoLintToken *> NoLintBegins;
+  SmallVector<NoLintBlockToken> CompletedBlocks;
+
+  for (const NoLintToken &NoLint : NoLints) {
+    // Check if a new block is being started. Add it to the stack.
+    if (NoLint.NoLintType == NoLintToken::Type::NoLintBegin)
+      NoLintBegins.emplace_back(&NoLint);
+    else if (NoLint.NoLintType == NoLintToken::Type::NoLintEnd) {
+      // The previous block is being closed. Pop one element off the stack.
+      if (NoLintBegins.empty() ||
+          !NoLintBlockToken::isValidPair(*NoLintBegins.back(), NoLint, SM)) {
+        // Trying to close the wrong 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);
+        ClangTidyError Error = createNolintError(Context, SM, NoLint);
+        Errors.emplace_back(Error);
+        continue;
       }
+      NoLintBlockToken Block{*NoLintBegins.back(), NoLint};
+      CompletedBlocks.emplace_back(Block);
+      NoLintBegins.pop_back();
     }
-    // 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.
+
+  // Check that every block has been closed by the end of the file.
+  for (const NoLintToken *NoLintBegin : NoLintBegins) {
+    ClangTidyError Error = createNolintError(Context, SM, *NoLintBegin);
+    Errors.emplace_back(Error);
   }
 
-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');
-  auto Error = tallyNolintBegins(Context, SM, CheckName, PrevLines,
-                                 FileStartLoc, NolintBegins);
-  if (Error) {
-    SuppressionErrors.emplace_back(Error.getValue());
-  }
-  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,
-                            NolintBegins);
-  if (Error) {
-    SuppressionErrors.emplace_back(Error.getValue());
-  }
+  return CompletedBlocks;
 
-  // 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 : NolintBegins) {
-    SuppressionErrors.emplace_back(
-        createNolintError(Context, SM, NolintBegin.first, true));
-  }
+const SmallVector<NoLintBlockToken> &ClangTidyContext::getNoLintBlocks(
+    FileID File, SmallVectorImpl<ClangTidyError> &Errors, bool AllowIO) const {
+  // Check cache first.
+  const auto Cache = NoLintCache.find(File);
+  if (Cache != NoLintCache.end())
+    return Cache->second;
+
+  // This file has not been searched yet. Warning: heavy operation below. Need
+  // to read the whole file.
+  auto &NewCache = NoLintCache.FindAndConstruct(File);
+  const SourceManager &SM = DiagEngine->getSourceManager();
+  Optional<StringRef> Buffer = getBuffer(SM, File, AllowIO);
+  if (Buffer) {
+    NewCache.second = collectNoLintBlocks(
+        *this, SM, *Buffer, SM.getLocForStartOfFile(File), Errors);
+  }
+  return NewCache.second;
 
-  return WithinNolintBegin && SuppressionErrors.empty();
+/// Look behind and ahead for '\n' characters. These mark the start and end of
+/// this line.
+static std::pair<size_t, size_t> getLineStartAndEnd(StringRef S, size_t From) {
+  size_t StartPos = S.find_last_of('\n', From) + 1;
+  size_t EndPos = std::min(S.find('\n', From), S.size());
+  return std::make_pair(StartPos, EndPos);
 }
 
 static bool
 lineIsMarkedWithNOLINT(const ClangTidyContext &Context,
                        SmallVectorImpl<ClangTidyError> &SuppressionErrors,
                        bool AllowIO, const SourceManager &SM,
                        SourceLocation Loc, StringRef CheckName,
                        bool EnableNolintBlocks) {
   // Get source code for this location.
+  Loc = SM.getSpellingLoc(Loc);
   FileID File;
   unsigned Offset;
-  std::tie(File, Offset) = SM.getDecomposedSpellingLoc(Loc);
+  std::tie(File, Offset) = SM.getDecomposedLoc(Loc);
   Optional<StringRef> Buffer = getBuffer(SM, File, AllowIO);
   if (!Buffer)
     return false;
 
   // Check if there's a NOLINT on this line.
-  StringRef TextAfterDiag = Buffer->substr(Offset);
-  StringRef RestOfThisLine, FollowingLines;
-  std::tie(RestOfThisLine, FollowingLines) = TextAfterDiag.split('\n');
-  if (isNOLINTFound("NOLINT", CheckName, RestOfThisLine))
+  auto ThisLinePos = getLineStartAndEnd(*Buffer, Offset);
+  StringRef ThisLine = Buffer->slice(ThisLinePos.first, ThisLinePos.second);
+  if (isNoLintFound(ThisLine, NoLintToken::Type::NoLint, CheckName))
     return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
-  StringRef TextBeforeDiag = Buffer->substr(0, Offset);
-  size_t LastNewLinePos = TextBeforeDiag.rfind('\n');
-  StringRef PrevLines = (LastNewLinePos == StringRef::npos)
-                            ? StringRef()
-                            : TextBeforeDiag.slice(0, LastNewLinePos);
-  LastNewLinePos = PrevLines.rfind('\n');
-  StringRef PrevLine = (LastNewLinePos == StringRef::npos)
-                           ? PrevLines
-                           : PrevLines.substr(LastNewLinePos + 1);
-  if (isNOLINTFound("NOLINTNEXTLINE", CheckName, PrevLine))
+  if (ThisLinePos.first > 0) {
+    auto PrevLinePos = getLineStartAndEnd(*Buffer, ThisLinePos.first - 1);
+    StringRef PrevLine = Buffer->slice(PrevLinePos.first, PrevLinePos.second);
+    if (isNoLintFound(PrevLine, NoLintToken::Type::NoLintNextLine, CheckName))
       return true;
+  }
 
   // Check if this line is within a NOLINT(BEGIN...END) block.
-  return EnableNolintBlocks &&
-         lineIsWithinNolintBegin(Context, SuppressionErrors, SM, Loc, CheckName,
-                                 TextBeforeDiag, TextAfterDiag);
+  if (!EnableNolintBlocks)
+    return false;
+  const SmallVector<NoLintBlockToken> &NoLintBlocks =
+      Context.getNoLintBlocks(File, SuppressionErrors, AllowIO);
+  return std::any_of(NoLintBlocks.begin(), NoLintBlocks.end(),
+                     [&](const NoLintBlockToken &NoLintBlock) {
+                       return NoLintBlock.suppresses(Loc, CheckName, SM);
+                     });
 }
 
 static bool lineIsMarkedWithNOLINTinMacro(
     const Diagnostic &Info, const ClangTidyContext &Context,
     SmallVectorImpl<ClangTidyError> &SuppressionErrors, bool AllowIO,
     bool EnableNolintBlocks) {
   const SourceManager &SM = Info.getSourceManager();
   SourceLocation Loc = Info.getLocation();
if (!Note.Fix.empty()) {
         // We have 2 different fixes in notes, bail out.
         return nullptr;
       Result = &Note.Fix;
     }
   }
   return Result;
 }
 
+NoLintToken::Type NoLintToken::strToType(StringRef S) {
+  static const llvm::StringMap<Type> StrMapping = {
+      {"NOLINT", Type::NoLint},
+      {"NOLINTNEXTLINE", Type::NoLintNextLine},
+      {"NOLINTBEGIN", Type::NoLintBegin},
+      {"NOLINTEND", Type::NoLintEnd}};
+  auto It = StrMapping.find(S);
+  if (It == StrMapping.end())
+    return Type::Unknown;
+  return It->second;
+}
+
+bool NoLintToken::suppresses(StringRef CheckName) const {
+  // NOLINT supports an optional list of checks.
+  // If the list is omitted, the default behavior is to suppresses all checks.
+  assert(SpecifiesChecks || Checks.empty());
+  if (!SpecifiesChecks)
+    return true;
+  GlobList Globs(Checks, /*KeepNegativeGlobs=*/false);
+  return Globs.contains(CheckName);
+}
+
+bool NoLintBlockToken::isValidPair(const NoLintToken &Begin,
+                                   const NoLintToken &End,
+                                   const SourceManager &SM) {
+  return (Begin.NoLintType == NoLintToken::Type::NoLintBegin) &&
+         (End.NoLintType == NoLintToken::Type::NoLintEnd) &&
+         (SM.getFileID(Begin.Loc) == SM.getFileID(End.Loc)) &&
+         (Begin.Loc < End.Loc) &&
+         (Begin.SpecifiesChecks == End.SpecifiesChecks) &&
+         (Begin.Checks == End.Checks);
+}
+
+bool NoLintBlockToken::contains(SourceLocation Loc,
+                                const SourceManager &SM) const {
+  if (!isValidPair(Begin, End, SM))
+    return false;
+  bool InSameFile = SM.getFileID(Loc) == SM.getFileID(Begin.Loc);
+  return InSameFile && (Begin.Loc < Loc) && (Loc < End.Loc);
+}
+
+bool NoLintBlockToken::suppresses(SourceLocation DiagLoc, StringRef DiagName,
+                                  const SourceManager &SM) const {
+  return contains(DiagLoc, SM) && Begin.suppresses(DiagName);
+}
+
 } // namespace tidy
 } // namespace clang
 
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
   if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
     return;
 
   SmallVector<ClangTidyError, 1> SuppressionErrors;
   if (shouldSuppressDiagnostic(DiagLevel, Info, Context, SuppressionErrors,
                                EnableNolintBlocks)) {
-    ++Context.Stats.ErrorsIgnoredNOLINT;
+    // Even though this diagnostic is suppressed, there may be other issues with
+    // the file that need raising, such as unclosed NOLINT(BEGIN/END) blocks.
+    Context.DiagEngine->Clear();
+    for (const auto &Error : SuppressionErrors)
+      Context.diag(Error);
     // Ignored a warning, should ignore related notes as well
+    ++Context.Stats.ErrorsIgnoredNOLINT;
     LastErrorWasIgnored = true;
     return;
   }
 
   LastErrorWasIgnored = false;
   // Count warnings/errors.
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
 

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

 class C3 { C3(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
 
 class C4 { C4(int i); }; // NOLINT(google-explicit-constructor)
 
 class C5 { C5(int i); }; // NOLINT(some-check, google-explicit-constructor)
 
 class C6 { C6(int i); }; // NOLINT without-brackets-skip-all, another-check
 
-class C7 { C7(int i); }; // NOLINTNEXTLINE doesn't get misconstrued as a NOLINT
+// Other NOLINT* types (e.g. NEXTLINE) should not be misconstrued as a NOLINT:
+class C7 { C7(int i); }; // NOLINTNEXTLINE
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
 
+// NOLINT must be UPPERCASE:
+class C8 { C8(int i); }; // nolint
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// Unrecognized marker:
+class C9 { C9(int i); }; // NOLINTXYZ
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// C-style comments are supported:
+class C10 { C10(int i); }; /* NOLINT */
+/* NOLINT */ class C11 { C11(int i); };
+
+// Multiple NOLINTs in the same comment:
+class C12 { C12(int i); }; // NOLINT(some-other-check) NOLINT(google-explicit-constructor)
+class C13 { C13(int i); }; // NOLINT(google-explicit-constructor) NOLINT(some-other-check)
+class C14 { C14(int i); }; // NOLINTNEXTLINE(some-other-check) NOLINT(google-explicit-constructor)
+
+// Any text after a NOLINT expression is treated as a comment:
+class C15 { C15(int i); }; // NOLINT: suppress check because <reason>
+class C16 { C16(int i); }; // NOLINT(google-explicit-constructor): suppress check because <reason>
+
 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); };

 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays]
 
 int array2[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays)
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
 
 int array3[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 23 warnings (23 NOLINT)
+// CHECK-MESSAGES: Suppressed 30 warnings (30 NOLINT)

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

+// RUN: not clang-tidy %s --checks='-*,google-explicit-constructor,google-readability-casting' 2>&1 | FileCheck %s
+
+// NOLINTBEGIN(google-explicit-constructor)
+// NOLINTBEGIN(google-readability-casting)
+class A { A(int i); };
+auto Num = (unsigned int)(-1);
+// NOLINTEND(google-explicit-constructor)
+// 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-10]]: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-10]]: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-all-end-glob.cpp

+// RUN: not clang-tidy %s --checks='-*,google-explicit-constructor' 2>&1 | FileCheck %s
+
+// 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-7]]:4: error: unmatched 'NOLIN
+// CHECK: TBEGIN' comment without a subsequent 'NOLIN
+// CHECK: TEND' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-9]]: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-all-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-7]]:4: error: unmatched 'NOLIN
+// CHECK: TBEGIN' comment without a subsequent 'NOLIN
+// CHECK: TEND' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-9]]: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-glob-end-all.cpp

+// RUN: not clang-tidy %s --checks="-*,google-explicit-constructor" 2>&1 | FileCheck %s
+
+// 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-7]]:4: error: unmatched 'NOLIN
+// CHECK: TBEGIN' comment without a subsequent 'NOLIN
+// CHECK: TEND' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-9]]: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-glob-end-specific.cpp

+// RUN: not clang-tidy %s --checks="-*,google-explicit-constructor" 2>&1 | FileCheck %s
+
+// NOLINTBEGIN(*)
+class B { B(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-7]]:4: error: unmatched 'NOLIN
+// CHECK: TBEGIN' comment without a subsequent 'NOLIN
+// CHECK: TEND' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-9]]: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-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-multiple-end-single.cpp

+// RUN: not clang-tidy %s --checks='-*,google-explicit-constructor,google-readability-casting' 2>&1 | FileCheck %s
+
+// 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-begin-single-end-multiple.cpp

+// RUN: not clang-tidy %s --checks='-*,google-explicit-constructor,google-readability-casting' 2>&1 | FileCheck %s
+
+// NOLINTBEGIN(google-explicit-constructor)
+// NOLINTBEGIN(google-readability-casting)
+class B { B(int i); };
+auto Num2 = (unsigned int)(-1);
+// NOLINTEND(google-explicit-constructor,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]]:4: error: unmatched 'NOLIN
+// CHECK: TBEGIN' comment without a subsequent 'NOLIN
+// CHECK: TEND' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-13]]:11: warning: single-argument constructors must be marked explicit
+// 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-begin-specific-end-all.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-7]]:4: error: unmatched 'NOLIN
+// CHECK: TBEGIN' comment without a subsequent 'NOLIN
+// CHECK: TEND' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-9]]: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-glob.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-7]]:4: error: unmatched 'NOLIN
+// CHECK: TBEGIN' comment without a subsequent 'NOLIN
+// CHECK: TEND' comment [clang-tidy-nolint]
+// CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-9]]: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-mismatched-delims.cpp

 // RUN: not clang-tidy %s --checks='-*,google-explicit-constructor' 2>&1 | FileCheck %s
 
 // NOLINTBEGIN
 // NOLINTBEGIN
 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-8]]:4: error: unmatched 'NOLIN
 // CHECK: TBEGIN' comment without a subsequent 'NOLIN
 // CHECK: TEND' comment [clang-tidy-nolint]
-// CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit

clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp

 // RUN: not clang-tidy %s --checks='-*,google-explicit-constructor' 2>&1 | FileCheck %s
 
 // NOLINTBEGIN(google-explicit-constructor)
 class A { A(int i); };
 // NOLINTEND(google-explicit-constructo) <-- typo: missing 'r' 
 
 // 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-7]]:4: error: unmatched 'NOLIN
 // CHECK: TBEGIN' comment without a subsequent 'NOLIN
 // CHECK: TEND' comment [clang-tidy-nolint]
 // CHECK: :[[@LINE-9]]:11: warning: single-argument constructors must be marked explicit
+// CHECK: :[[@LINE-9]]: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

 
 // NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN(some-other-check)
 class C6 { C6(int i); };
 // NOLINTEND(some-other-check)
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN(google-explicit-constructor)
-// NOLINTBEGIN(some-other-check)
-class C7 { C7(int i); };
-// NOLINTEND(google-explicit-constructor)
-// NOLINTEND(some-other-check)
-
-// NOLINTBEGIN(google-explicit-constructor)
 // NOLINTBEGIN
-class C8 { C8(int i); };
+class C7 { C7(int i); };
 // NOLINTEND
 // NOLINTEND(google-explicit-constructor)
 
 // NOLINTBEGIN
 // NOLINTBEGIN(google-explicit-constructor)
-class C9 { C9(int i); };
+class C8 { C8(int i); };
 // NOLINTEND(google-explicit-constructor)
 // NOLINTEND
 
 // NOLINTBEGIN(not-closed-bracket-is-treated-as-skip-all
-class C10 { C10(int i); };
+class C9 { C9(int i); };
 // NOLINTEND(not-closed-bracket-is-treated-as-skip-all
 
 // NOLINTBEGIN without-brackets-skip-all, another-check
-class C11 { C11(int i); };
+class C10 { C10(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
 
 // NOLINTBEGIN(google*)
-class C12 { C12(int i); };
+class C11 { C11(int i); };
 // NOLINTEND(google*)
 
 // NOLINTBEGIN(*explicit-constructor)
-class C15 { C15(int i); };
+class C12 { C12(int i); };
 // NOLINTEND(*explicit-constructor)
 
 // NOLINTBEGIN(*explicit*)
-class C16 { C16(int i); };
+class C13 { C13(int i); };
 // NOLINTEND(*explicit*)
 
 // NOLINTBEGIN(-explicit-constructor)
-class C17 { C17(int x); };
+class C14 { C14(int x); };
 // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
 // NOLINTEND(-explicit-constructor)
 
 // NOLINTBEGIN(google*,-google*)
-class C18 { C18(int x); };
+class C15 { C15(int x); };
 // NOLINTEND(google*,-google*)
 
 // NOLINTBEGIN(*,-google*)
-class C19 { C19(int x); };
+class C16 { C16(int x); };
 // NOLINTEND(*,-google*)
 
 int array1[10];
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays]
 
 // NOLINTBEGIN(cppcoreguidelines-avoid-c-arrays)
 int array2[10];
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead [modernize-avoid-c-arrays]
 // NOLINTEND(cppcoreguidelines-avoid-c-arrays)
 
 // NOLINTBEGIN(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array3[10];
 // NOLINTEND(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 
 // NOLINTBEGIN(*-avoid-c-arrays)
 int array4[10];
 // NOLINTEND(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 27 warnings (27 NOLINT).
+// CHECK-MESSAGES: Suppressed 26 warnings (26 NOLINT).