[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

198

this change looks unrelated to the current patch.

318–319

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.

341

nit: there's also llvm::StringSwitch

351

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

358

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

375

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

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

67

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.

77

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

95

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

211

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

233

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

198

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.

351

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

77

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

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

117

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

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

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

117

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

161

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

78

why are we moving these two parameters here?

99

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

233–234

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

2

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

2

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

322

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

322

Tab with spaces

354

Nit: tab with spaces

356

Nit: tab with spaces

358

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

358

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

380

Don't we usually use SourceLocation objects for this?

[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

78

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

380

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

350

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

214

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

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

90

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.

134

nit: static constexpr llvm::StringLiteral

142

nit: break instead?

157

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

171

nit: you can drop the NoLintToken

428

no need for class here.

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

42

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

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/CMakeLists.txt

add_clang_library(clangTidy
   ClangTidy.cpp
   ClangTidyCheck.cpp
   ClangTidyModule.cpp
   ClangTidyDiagnosticConsumer.cpp
   ClangTidyOptions.cpp
   ClangTidyProfiling.cpp
   ExpandModularHeadersPPCallbacks.cpp
   GlobList.cpp
+  NoLintDirectiveHandler.cpp
 
   DEPENDS
   ClangSACheckers
   omp_gen
   )
 
 clang_target_link_libraries(clangTidy
   PRIVATE

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

 //===--- ClangTidyDiagnosticConsumer.h - clang-tidy -------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H
 
 #include "ClangTidyOptions.h"
 #include "ClangTidyProfiling.h"
+#include "NoLintDirectiveHandler.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Regex.h"
 
 namespace clang {
 
 class ASTContext;
unsigned errorsIgnored() const {
     return ErrorsIgnoredNOLINT + ErrorsIgnoredCheckFilter +
            ErrorsIgnoredNonUserCode + ErrorsIgnoredLineFilter;
   }
 };
 
 /// 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);
+  DiagnosticBuilder diag(const tooling::Diagnostic &Error);
 
   /// Report any errors to do with reading the configuration using this method.
   DiagnosticBuilder
   configurationDiag(StringRef Message,
                     DiagnosticIDs::Level Level = DiagnosticIDs::Warning);
 
+  /// 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 any NOLINT is malformed, e.g. a BEGIN without a subsequent END, an
+  /// error about it will be returned in output \param NoLintErrors.
+  /// If \param 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.
+  bool
+  shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                           const Diagnostic &Info,
+                           SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
+                           bool AllowIO = true, bool EnableNoLintBlocks = true);
+
   /// 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);
 
return DiagLevelAndFormatString(
         std::string(
             DiagEngine->getDiagnosticIDs()->getDescription(DiagnosticID)));
   }
 
 private:
   // Writes to Stats.
   friend class ClangTidyDiagnosticConsumer;
 
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;
   ClangTidyOptions CurrentOptions;
 
   std::unique_ptr<CachedGlobList> CheckFilter;
   std::unique_ptr<CachedGlobList> WarningAsErrorFilter;
 
   LangOptions LangOpts;
 
   ClangTidyStats Stats;
 
   std::string CurrentBuildDirectory;
 
   llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
 
   bool Profile;
   std::string ProfilePrefix;
 
   bool AllowEnablingAnalyzerAlphaCheckers;
-};
 
-/// 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);
+  NoLintDirectiveHandler NoLintHandler;
+};
 
 /// 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

 ///    http://clang.llvm.org/docs/HowToSetupToolingForLLVM.html
 ///  for details on setting it up with LLVM source tree.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyOptions.h"
 #include "GlobList.h"
+#include "NoLintDirectiveHandler.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTDiagnostic.h"
 #include "clang/AST/Attr.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/DiagnosticRenderer.h"

 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(
     StringRef CheckName, StringRef Description,
     DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
   unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID(
       Level, (Description + " [" + CheckName + "]").str());
   CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
   return DiagEngine->Report(ID);
 }
 
-DiagnosticBuilder ClangTidyContext::diag(const ClangTidyError &Error) {
+DiagnosticBuilder ClangTidyContext::diag(const tooling::Diagnostic &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(
       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);
 }
 
+bool ClangTidyContext::shouldSuppressDiagnostic(
+    DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info,
+    SmallVectorImpl<tooling::Diagnostic> &NoLintErrors, bool AllowIO,
+    bool EnableNoLintBlocks) {
+  std::string CheckName = getCheckName(Info.getID());
+  return NoLintHandler.shouldSuppress(DiagLevel, Info, CheckName, NoLintErrors,
+                                      AllowIO, EnableNoLintBlocks);
+}
+
 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);
if (Error.DiagnosticName == "clang-tidy-config") {
       Errors.pop_back();
     } 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;
-    }
-  }
-
-  if (FoundNolintIndex)
-    *FoundNolintIndex = NolintIndex;
-
-  return true;
-}
-
-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) {
-  ClangTidyError Error("clang-tidy-nolint", ClangTidyError::Error,
-                       Context.getCurrentBuildDirectory(), false);
-  StringRef Message =
-      IsNolintBegin
-          ? ("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);
-  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
-        // 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');
-  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());
-  }
-
-  // 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));
-  }
-
-  return WithinNolintBegin && SuppressionErrors.empty();
-}
-
-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.
-  FileID File;
-  unsigned Offset;
-  std::tie(File, Offset) = SM.getDecomposedSpellingLoc(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))
-    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))
-    return true;
-
-  // Check if this line is within a NOLINT(BEGIN...END) block.
-  return EnableNolintBlocks &&
-         lineIsWithinNolintBegin(Context, SuppressionErrors, SM, Loc, CheckName,
-                                 TextBeforeDiag, TextAfterDiag);
-}
-
-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();
-  std::string CheckName = Context.getCheckName(Info.getID());
-  while (true) {
-    if (lineIsMarkedWithNOLINT(Context, SuppressionErrors, AllowIO, SM, Loc,
-                               CheckName, EnableNolintBlocks))
-      return true;
-    if (!Loc.isMacroID())
-      return false;
-    Loc = SM.getImmediateExpansionRange(Loc).getBegin();
-  }
-  return false;
-}
-
 namespace clang {
 namespace tidy {
 
-bool shouldSuppressDiagnostic(
-    DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info,
-    ClangTidyContext &Context,
-    SmallVectorImpl<ClangTidyError> &SuppressionErrors, bool AllowIO,
-    bool EnableNolintBlocks) {
-  return Info.getLocation().isValid() &&
-         DiagLevel != DiagnosticsEngine::Error &&
-         DiagLevel != DiagnosticsEngine::Fatal &&
-         lineIsMarkedWithNOLINTinMacro(Info, Context, SuppressionErrors,
-                                       AllowIO, EnableNolintBlocks);
-}
-
 const llvm::StringMap<tooling::Replacements> *
 getFixIt(const tooling::Diagnostic &Diagnostic, bool GetFixFromNotes) {
   if (!Diagnostic.Message.Fix.empty())
     return &Diagnostic.Message.Fix;
   if (!GetFixFromNotes)
     return nullptr;
   const llvm::StringMap<tooling::Replacements> *Result = nullptr;
   for (const auto &Note : Diagnostic.Notes) {
     if (!Note.Fix.empty()) {
       if (Result)
         // We have 2 different fixes in notes, bail out.
         return nullptr;
       Result = &Note.Fix;
     }
   }
   return Result;
 }
 
 } // 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,
+  SmallVector<tooling::Diagnostic, 1> SuppressionErrors;
+  if (Context.shouldSuppressDiagnostic(DiagLevel, Info, SuppressionErrors,
                                        EnableNolintBlocks)) {
     ++Context.Stats.ErrorsIgnoredNOLINT;
     // Ignored a warning, should ignore related notes as well
     LastErrorWasIgnored = true;
+    Context.DiagEngine->Clear();
+    for (const auto &Error : SuppressionErrors)
+      Context.diag(Error);
     return;
   }
 
   LastErrorWasIgnored = false;
   // Count warnings/errors.
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
 
   if (DiagLevel == DiagnosticsEngine::Note) {
     assert(!Errors.empty() &&
            "A diagnostic note can only be appended to a message.");
   } else {
     finalizeLastError();
     std::string CheckName = Context.getCheckName(Info.getID());
     if (CheckName.empty()) {
       // This is a compiler diagnostic without a warning option. Assign check
       // name based on its level.
       switch (DiagLevel) {
       case DiagnosticsEngine::Error:
       case DiagnosticsEngine::Fatal:
         CheckName = "clang-diagnostic-error";
         break;
       case DiagnosticsEngine::Warning:
         CheckName = "clang-diagnostic-warning";
         break;
       case DiagnosticsEngine::Remark:
         CheckName = "clang-diagnostic-remark";
         break;
       default:
         CheckName = "clang-diagnostic-unknown";
         break;
       }
     }

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

+//===-- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h ----*- C++ *-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_NOLINTDIRECTIVEHANDLER_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_NOLINTDIRECTIVEHANDLER_H
+
+#include "clang/Basic/Diagnostic.h"
+#include "llvm/ADT/StringRef.h"
+#include <memory>
+
+namespace clang {
+namespace tooling {
+struct Diagnostic;
+} // namespace tooling
+} // namespace clang
+
+namespace llvm {
+template <typename T> class SmallVectorImpl;
+} // namespace llvm
+
+namespace clang {
+namespace tidy {
+
+/// This class is used to locate NOLINT comments in the file being analyzed, to
+/// decide whether a diagnostic should be suppressed.
+/// This class keeps a cache of every NOLINT comment found so that files do not
+/// have to be repeatedly parsed each time a new diagnostic is raised.
+class NoLintDirectiveHandler {
+public:
+  NoLintDirectiveHandler();
+  ~NoLintDirectiveHandler();
+
+  bool shouldSuppress(DiagnosticsEngine::Level DiagLevel,
+                      const Diagnostic &Diag, llvm::StringRef DiagName,
+                      llvm::SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
+                      bool AllowIO, bool EnableNoLintBlocks);
+
+private:
+  class Impl;
+  std::unique_ptr<Impl> PImpl;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_NOLINTDIRECTIVEHANDLER_H

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

+//===-- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp -----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+///
+///  \file This file implements the NoLintDirectiveHandler class, which is used
+///  to locate NOLINT comments in the file being analyzed, to decide whether a
+///  diagnostic should be suppressed.
+///
+//===----------------------------------------------------------------------===//
+
+#include "NoLintDirectiveHandler.h"
+#include "GlobList.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Tooling/Core/Diagnostic.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringSwitch.h"
+#include <cassert>
+#include <cstddef>
+#include <iterator>
+#include <string>
+#include <tuple>
+#include <type_traits>
+#include <utility>
+
+namespace clang {
+namespace tidy {
+
+//===----------------------------------------------------------------------===//
+// NoLintType
+//===----------------------------------------------------------------------===//
+
+// The type - one of NOLINT[NEXTLINE/BEGIN/END].
+enum class NoLintType { NoLint, NoLintNextLine, NoLintBegin, NoLintEnd };
+
+// Convert a string like "NOLINTNEXTLINE" to its enum `Type::NoLintNextLine`.
+// Return `None` if the string is unrecognized.
+static Optional<NoLintType> strToNoLintType(StringRef Str) {
+  auto Type = llvm::StringSwitch<Optional<NoLintType>>(Str)
+                  .Case("NOLINT", NoLintType::NoLint)
+                  .Case("NOLINTNEXTLINE", NoLintType::NoLintNextLine)
+                  .Case("NOLINTBEGIN", NoLintType::NoLintBegin)
+                  .Case("NOLINTEND", NoLintType::NoLintEnd)
+                  .Default(None);
+  return Type;
+}
+
+//===----------------------------------------------------------------------===//
+// NoLintToken
+//===----------------------------------------------------------------------===//
+
+// Whitespace within a NOLINT's check list shall be ignored.
+// "NOLINT( check1, check2 )" is equivalent to "NOLINT(check1,check2)".
+// Return the check list with all extraneous whitespace removed.
+static std::string trimWhitespace(StringRef Checks) {
+  SmallVector<StringRef> Split;
+  Checks.split(Split, ',');
+  for (StringRef &Check : Split)
+    Check = Check.trim();
+  return llvm::join(Split, ",");
+}
+
+namespace {
+
+// Record the presence of a NOLINT comment - its type, location, checks -
+// as parsed from the file's character contents.
+class NoLintToken {
+public:
+  // \param Checks:
+  // - If unspecified (i.e. `None`) then ALL checks are suppressed - equivalent
+  //   to NOLINT(*).
+  // - An empty string means nothing is suppressed - equivalent to NOLINT().
+  // - Negative globs ignored (which would effectively disable the suppression).
+  NoLintToken(NoLintType Type, size_t Pos, const Optional<std::string> &Checks)
+      : Type(Type), Pos(Pos), ChecksGlob(std::make_unique<CachedGlobList>(
+                                  Checks.getValueOr("*"),
+                                  /*KeepNegativeGlobs=*/false)) {
+    if (Checks)
+      this->Checks = trimWhitespace(*Checks);
+  }
+
+  // The type - one of NOLINT[NEXTLINE/BEGIN/END].
+  NoLintType Type;
+
+  // The location of the first character, "N", in "NOLINT".
+  size_t Pos;
+
+  // If this NOLINT specifies checks, return the checks.
+  Optional<std::string> checks() const { return Checks; }
+
+  // Whether this NOLINT applies to the provided check.
+  bool suppresses(StringRef Check) const { return ChecksGlob->contains(Check); }
+
+private:
+  Optional<std::string> Checks;
+  std::unique_ptr<CachedGlobList> ChecksGlob;
+};
+
+} // namespace
+
+// Consume the entire buffer and return all `NoLintToken`s that were found.
+static SmallVector<NoLintToken> getNoLints(StringRef Buffer) {
+  static constexpr llvm::StringLiteral NOLINT = "NOLINT";
+  SmallVector<NoLintToken> NoLints;
+
+  size_t Pos = 0;
+  while (Pos < Buffer.size()) {
+    // Find NOLINT:
+    const size_t NoLintPos = Buffer.find(NOLINT, Pos);
+    if (NoLintPos == StringRef::npos)
+      break; // Buffer exhausted
+
+    // Read [A-Z] characters immediately after "NOLINT", e.g. the "NEXTLINE" in
+    // "NOLINTNEXTLINE".
+    Pos = NoLintPos + NOLINT.size();
+    while (Pos < Buffer.size() && llvm::isAlpha(Buffer[Pos]))
+      ++Pos;
+
+    // Is this a recognized NOLINT type?
+    const Optional<NoLintType> NoLintType =
+        strToNoLintType(Buffer.slice(NoLintPos, Pos));
+    if (!NoLintType)
+      continue;
+
+    // Get checks, if specified.
+    Optional<std::string> Checks;
+    if (Pos < Buffer.size() && Buffer[Pos] == '(') {
+      size_t ClosingBracket = Buffer.find_first_of("\n)", ++Pos);
+      if (ClosingBracket != StringRef::npos && Buffer[ClosingBracket] == ')') {
+        Checks = Buffer.slice(Pos, ClosingBracket).str();
+        Pos = ClosingBracket + 1;
+      }
+    }
+
+    NoLints.emplace_back(*NoLintType, NoLintPos, Checks);
+  }
+
+  return NoLints;
+}
+
+//===----------------------------------------------------------------------===//
+// NoLintBlockToken
+//===----------------------------------------------------------------------===//
+
+namespace {
+
+// Represents a source range within a pair of NOLINT(BEGIN/END) comments.
+class NoLintBlockToken {
+public:
+  NoLintBlockToken(NoLintToken Begin, const NoLintToken &End)
+      : Begin(std::move(Begin)), EndPos(End.Pos) {
+    assert(this->Begin.Type == NoLintType::NoLintBegin);
+    assert(End.Type == NoLintType::NoLintEnd);
+    assert(this->Begin.Pos < End.Pos);
+    assert(this->Begin.checks() == End.checks());
+  }
+
+  // Whether the provided diagnostic is within and is suppressible by this block
+  // of NOLINT(BEGIN/END) comments.
+  bool suppresses(size_t DiagPos, StringRef DiagName) const {
+    return (Begin.Pos < DiagPos) && (DiagPos < EndPos) &&
+           Begin.suppresses(DiagName);
+  }
+
+private:
+  NoLintToken Begin;
+  size_t EndPos;
+};
+
+} // namespace
+
+// Match NOLINTBEGINs with their corresponding NOLINTENDs and move them into
+// `NoLintBlockToken`s. If any BEGINs or ENDs are left over, they are moved to
+// `UnmatchedTokens`.
+static SmallVector<NoLintBlockToken>
+formNoLintBlocks(SmallVector<NoLintToken> &NoLints,
+                 SmallVectorImpl<NoLintToken> &UnmatchedTokens) {
+  SmallVector<NoLintBlockToken> CompletedBlocks;
+  SmallVector<NoLintToken> Stack;
+
+  // Nested blocks must be fully contained within their parent block. What this
+  // means is that when you have a series of nested BEGIN tokens, the END tokens
+  // shall appear in the reverse order, starting with the closing of the
+  // inner-most block first, then the next level up, and so on. This is
+  // essentially a last-in-first-out/stack system.
+  for (NoLintToken &NoLint : NoLints) {
+    if (NoLint.Type == NoLintType::NoLintBegin)
+      // A new block is being started. Add it to the stack.
+      Stack.emplace_back(std::move(NoLint));
+    else if (NoLint.Type == NoLintType::NoLintEnd) {
+      if (!Stack.empty() && Stack.back().checks() == NoLint.checks())
+        // The previous block is being closed. Pop one element off the stack.
+        CompletedBlocks.emplace_back(std::move(Stack.pop_back_val()), NoLint);
+      else
+        // Trying to close the wrong block.
+        UnmatchedTokens.emplace_back(std::move(NoLint));
+    }
+  }
+
+  llvm::move(Stack, std::back_inserter(UnmatchedTokens));
+  return CompletedBlocks;
+}
+
+//===----------------------------------------------------------------------===//
+// NoLintDirectiveHandler::Impl
+//===----------------------------------------------------------------------===//
+
+class NoLintDirectiveHandler::Impl {
+public:
+  bool shouldSuppress(DiagnosticsEngine::Level DiagLevel,
+                      const Diagnostic &Diag, StringRef DiagName,
+                      SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
+                      bool AllowIO, bool EnableNoLintBlocks);
+
+private:
+  bool diagHasNoLintInMacro(const Diagnostic &Diag, StringRef DiagName,
+                            SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
+                            bool AllowIO, bool EnableNoLintBlocks);
+
+  bool diagHasNoLint(StringRef DiagName, SourceLocation DiagLoc,
+                     const SourceManager &SrcMgr,
+                     SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
+                     bool AllowIO, bool EnableNoLintBlocks);
+
+  void generateCache(const SourceManager &SrcMgr, StringRef FileName,
+                     FileID File, StringRef Buffer,
+                     SmallVectorImpl<tooling::Diagnostic> &NoLintErrors);
+
+  llvm::StringMap<SmallVector<NoLintBlockToken>> Cache;
+};
+
+bool NoLintDirectiveHandler::Impl::shouldSuppress(
+    DiagnosticsEngine::Level DiagLevel, const Diagnostic &Diag,
+    StringRef DiagName, SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
+    bool AllowIO, bool EnableNoLintBlocks) {
+  if (DiagLevel >= DiagnosticsEngine::Error)
+    return false;
+  return diagHasNoLintInMacro(Diag, DiagName, NoLintErrors, AllowIO,
+                              EnableNoLintBlocks);
+}
+
+// Look at the macro's spelling location for a NOLINT. If none is found, keep
+// looking up the call stack.
+bool NoLintDirectiveHandler::Impl::diagHasNoLintInMacro(
+    const Diagnostic &Diag, StringRef DiagName,
+    SmallVectorImpl<tooling::Diagnostic> &NoLintErrors, bool AllowIO,
+    bool EnableNoLintBlocks) {
+  SourceLocation DiagLoc = Diag.getLocation();
+  if (DiagLoc.isInvalid())
+    return false;
+  const SourceManager &SrcMgr = Diag.getSourceManager();
+  while (true) {
+    if (diagHasNoLint(DiagName, DiagLoc, SrcMgr, NoLintErrors, AllowIO,
+                      EnableNoLintBlocks))
+      return true;
+    if (!DiagLoc.isMacroID())
+      return false;
+    DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+  }
+  return false;
+}
+
+// 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 Buffer,
+                                                    size_t From) {
+  size_t StartPos = Buffer.find_last_of('\n', From) + 1;
+  size_t EndPos = std::min(Buffer.find('\n', From), Buffer.size());
+  return std::make_pair(StartPos, EndPos);
+}
+
+// Whether the line has a NOLINT of type = `Type` that can suppress the
+// diagnostic `DiagName`.
+static bool lineHasNoLint(StringRef Buffer,
+                          std::pair<size_t, size_t> LineStartAndEnd,
+                          NoLintType Type, StringRef DiagName) {
+  // Get all NOLINTs on the line.
+  Buffer = Buffer.slice(LineStartAndEnd.first, LineStartAndEnd.second);
+  SmallVector<NoLintToken> NoLints = getNoLints(Buffer);
+
+  // Do any of these NOLINTs match the desired type and diag name?
+  return llvm::any_of(NoLints, [&](const NoLintToken &NoLint) {
+    return NoLint.Type == Type && NoLint.suppresses(DiagName);
+  });
+}
+
+// Whether the provided diagnostic is located within and is suppressible by a
+// block of NOLINT(BEGIN/END) comments.
+static bool withinNoLintBlock(ArrayRef<NoLintBlockToken> NoLintBlocks,
+                              size_t DiagPos, StringRef DiagName) {
+  return llvm::any_of(NoLintBlocks, [&](const NoLintBlockToken &NoLintBlock) {
+    return NoLintBlock.suppresses(DiagPos, DiagName);
+  });
+}
+
+// Get the file contents as a string.
+static Optional<StringRef> getBuffer(const SourceManager &SrcMgr, FileID File,
+                                     bool AllowIO) {
+  return AllowIO ? SrcMgr.getBufferDataOrNone(File)
+                 : SrcMgr.getBufferDataIfLoaded(File);
+}
+
+// We will check for NOLINTs and NOLINTNEXTLINEs first. Checking for these is
+// not so expensive (just need to parse the current and previous lines). Only if
+// that fails do we look for NOLINT(BEGIN/END) blocks (which requires reading
+// the entire file).
+bool NoLintDirectiveHandler::Impl::diagHasNoLint(
+    StringRef DiagName, SourceLocation DiagLoc, const SourceManager &SrcMgr,
+    SmallVectorImpl<tooling::Diagnostic> &NoLintErrors, bool AllowIO,
+    bool EnableNoLintBlocks) {
+  // Translate the diagnostic's SourceLocation to a raw file + offset pair.
+  FileID File;
+  unsigned int Pos = 0;
+  std::tie(File, Pos) = SrcMgr.getDecomposedSpellingLoc(DiagLoc);
+
+  // We will only see NOLINTs in user-authored sources. No point reading the
+  // file if it is a <built-in>.
+  Optional<StringRef> FileName = SrcMgr.getNonBuiltinFilenameForID(File);
+  if (!FileName)
+    return false;
+
+  // Get file contents.
+  Optional<StringRef> Buffer = getBuffer(SrcMgr, File, AllowIO);
+  if (!Buffer)
+    return false;
+
+  // Check if there's a NOLINT on this line.
+  auto ThisLine = getLineStartAndEnd(*Buffer, Pos);
+  if (lineHasNoLint(*Buffer, ThisLine, NoLintType::NoLint, DiagName))
+    return true;
+
+  // Check if there's a NOLINTNEXTLINE on the previous line.
+  if (ThisLine.first > 0) {
+    auto PrevLine = getLineStartAndEnd(*Buffer, ThisLine.first - 1);
+    if (lineHasNoLint(*Buffer, PrevLine, NoLintType::NoLintNextLine, DiagName))
+      return true;
+  }
+
+  // Check if this line is within a NOLINT(BEGIN/END) block.
+  if (!EnableNoLintBlocks)
+    return false;
+
+  // Do we have cached NOLINT block locations for this file?
+  if (Cache.count(*FileName) == 0)
+    // Warning: heavy operation - need to read entire file.
+    generateCache(SrcMgr, *FileName, File, *Buffer, NoLintErrors);
+
+  return withinNoLintBlock(Cache[*FileName], Pos, DiagName);
+}
+
+// Construct a [clang-tidy-nolint] diagnostic to do with the unmatched
+// NOLINT(BEGIN/END) pair.
+static tooling::Diagnostic makeNoLintError(const SourceManager &SrcMgr,
+                                           FileID File,
+                                           const NoLintToken &NoLint) {
+  tooling::Diagnostic Error;
+  Error.DiagLevel = tooling::Diagnostic::Error;
+  Error.DiagnosticName = "clang-tidy-nolint";
+  StringRef Message =
+      (NoLint.Type == NoLintType::NoLintBegin)
+          ? ("unmatched 'NOLINTBEGIN' comment without a subsequent 'NOLINT"
+             "END' comment")
+          : ("unmatched 'NOLINTEND' comment without a previous 'NOLINT"
+             "BEGIN' comment");
+  SourceLocation Loc = SrcMgr.getComposedLoc(File, NoLint.Pos);
+  Error.Message = tooling::DiagnosticMessage(Message, SrcMgr, Loc);
+  return Error;
+}
+
+// Find all NOLINT(BEGIN/END) blocks in a file and store in the cache.
+void NoLintDirectiveHandler::Impl::generateCache(
+    const SourceManager &SrcMgr, StringRef FileName, FileID File,
+    StringRef Buffer, SmallVectorImpl<tooling::Diagnostic> &NoLintErrors) {
+  // Read entire file to get all NOLINTs.
+  SmallVector<NoLintToken> NoLints = getNoLints(Buffer);
+
+  // Match each BEGIN with its corresponding END.
+  SmallVector<NoLintToken> UnmatchedTokens;
+  Cache[FileName] = formNoLintBlocks(NoLints, UnmatchedTokens);
+
+  // Raise error for any BEGIN/END left over.
+  for (const NoLintToken &NoLint : UnmatchedTokens)
+    NoLintErrors.emplace_back(makeNoLintError(SrcMgr, File, NoLint));
+}
+
+//===----------------------------------------------------------------------===//
+// NoLintDirectiveHandler
+//===----------------------------------------------------------------------===//
+
+NoLintDirectiveHandler::NoLintDirectiveHandler()
+    : PImpl(std::make_unique<Impl>()) {}
+
+NoLintDirectiveHandler::~NoLintDirectiveHandler() = default;
+
+bool NoLintDirectiveHandler::shouldSuppress(
+    DiagnosticsEngine::Level DiagLevel, const Diagnostic &Diag,
+    StringRef DiagName, SmallVectorImpl<tooling::Diagnostic> &NoLintErrors,
+    bool AllowIO, bool EnableNoLintBlocks) {
+  return PImpl->shouldSuppress(DiagLevel, Diag, DiagName, NoLintErrors, AllowIO,
+                               EnableNoLintBlocks);
+}
+
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clangd/ParsedAST.cpp

 //===--- ParsedAST.cpp -------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ParsedAST.h"
 #include "../clang-tidy/ClangTidyCheck.h"
 #include "../clang-tidy/ClangTidyDiagnosticConsumer.h"
 #include "../clang-tidy/ClangTidyModuleRegistry.h"
+#include "../clang-tidy/NoLintDirectiveHandler.h"
 #include "AST.h"
 #include "Compiler.h"
 #include "Config.h"
 #include "Diagnostics.h"
 #include "Feature.h"
 #include "FeatureModule.h"
 #include "Headers.h"
 #include "HeuristicResolver.h"
ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
           // in the main file, because we don't want that function to query the
           // source buffer for preamble files. For the same reason, we ask
           // shouldSuppressDiagnostic to avoid I/O.
           // We let suppression comments take precedence over warning-as-error
           // to match clang-tidy's behaviour.
           bool IsInsideMainFile =
               Info.hasSourceManager() &&
               isInsideMainFile(Info.getLocation(), Info.getSourceManager());
-          SmallVector<tidy::ClangTidyError, 1> TidySuppressedErrors;
-          if (IsInsideMainFile &&
-              tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
-                                             TidySuppressedErrors,
-                                             /*AllowIO=*/false,
-                                             /*EnableNolintBlocks=*/false)) {
+          SmallVector<tooling::Diagnostic, 1> TidySuppressedErrors;
+          if (IsInsideMainFile && CTContext->shouldSuppressDiagnostic(
+                                      DiagLevel, Info, TidySuppressedErrors,
             // FIXME: should we expose the suppression error (invalid use of
             // NOLINT comments)?
             return DiagnosticsEngine::Ignored;
           }
 
           // Check for warning-as-error.
           if (DiagLevel == DiagnosticsEngine::Warning &&
               CTContext->treatAsError(CheckName)) {

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

+// NOLINTBEGIN
+class A { A(int i); };
+// NOLINTEND
+
+class B { B(int i); };

clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp

+
+class A { A(int i); };
+
+// NOLINTBEGIN
+class B { B(int i); };
+// NOLINTEND

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

 class C2 { C2(int i); }; // NOLINT(*)
 
 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 C6 { C6(int i); }; // NOLINT without-brackets-skip-all
 
-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:
+// NOLINTnextline
+class C8 { C8(int i); }; // nolint
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: single-argument constructors must be marked explicit
+
+// Unrecognized marker:
+// NOLINTNEXTLINEXYZ
+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)
+
+// NOLINTNEXTLINE(google-explicit-constructor) NOLINT(some-other-check)
+class C15 { C15(int i); }; 
+
+// Any text after a NOLINT expression is treated as a comment:
+class C16 { C16(int i); }; // NOLINT: suppress check because <reason>
+class C17 { C17(int i); }; // NOLINT(google-explicit-constructor): suppress check because <reason>
+
+// NOLINT must appear in its entirety on one line:
+class C18 { C18(int i); }; /* NOL
+INT */
+// CHECK-MESSAGES: :[[@LINE-2]]:13: warning: single-argument constructors must be marked explicit
+
+/* NO
+LINT */ class C19 { C19(int i); };
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: single-argument constructors must be marked explicit
+
+// Spaces between items in the comma-separated check list are ignroed:
+class C20 { C20(int i); }; // NOLINT( google-explicit-constructor )
+class C21 { C21(int i); }; // NOLINT( google-explicit-constructor , some-other-check )
+class C22 { C22(int i); }; // NOLINT(google-explicit- constructor)
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: single-argument constructors must be marked explicit
+
+// If there is a space between "NOLINT" and the bracket, it is treated as a regular NOLINT: 
+class C23 { C23(int i); }; // NOLINT (some-other-check)
+
 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 34 warnings (34 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-multiple-TUs.cpp

+// RUN: clang-tidy %S/Inputs/nolintbeginend/1st-translation-unit.cpp %S/Inputs/nolintbeginend/2nd-translation-unit.cpp --checks='-*,google-explicit-constructor' 2>&1 | FileCheck %s
+
+// CHECK-NOT: 1st-translation-unit.cpp:2:11: warning: single-argument constructors must be marked explicit
+// CHECK: 1st-translation-unit.cpp:5:11: warning: single-argument constructors must be marked explicit
+// CHECK: 2nd-translation-unit.cpp:2:11: warning: single-argument constructors must be marked explicit
+// CHECK-NOT: 2nd-translation-unit.cpp:5: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).