This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by salman-javed-nz on Dec 20 2021, 10:26 PM.

Details

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.

Diff Detail

Event Timeline

salman-javed-nz requested review of this revision.Dec 20 2021, 10:26 PM
salman-javed-nz created this revision.
salman-javed-nz created this object with visibility "salman-javed-nz (Salman Javed)".
salman-javed-nz created this object with edit policy "salman-javed-nz (Salman Javed)".
salman-javed-nz retitled this revision from [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks to *WIP* [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks.
salman-javed-nz edited the summary of this revision. (Show Details)
salman-javed-nz edited the summary of this revision. (Show Details)Dec 20 2021, 10:29 PM
salman-javed-nz edited the summary of this revision. (Show Details)
salman-javed-nz changed the visibility from "salman-javed-nz (Salman Javed)" to "Public (No Login Required)".Dec 20 2021, 11:03 PM
salman-javed-nz retitled this revision from *WIP* [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks to [clang-tidy] Performance improvements for NOLINTBEGIN/END blocks.Dec 20 2021, 11:27 PM
salman-javed-nz edited the summary of this revision. (Show Details)

Remove unnecessary llvm:: qualification.

salman-javed-nz changed the edit policy from "salman-javed-nz (Salman Javed)" to "All Users".Dec 21 2021, 12:37 PM

Updated the review's edit permissions. Sorry about that, @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
199

this change looks unrelated to the current patch.

319–320

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.

342

nit: there's also llvm::StringSwitch

352

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

359

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

376

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?

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
199

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.

352

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 planned changes to this revision.Dec 28 2021, 1:29 AM
salman-javed-nz edited the summary of this revision. (Show Details)Jan 6 2022, 8:58 PM
salman-javed-nz edited the summary of this revision. (Show Details)Jan 6 2022, 9:03 PM
salman-javed-nz updated this revision to Diff 398048.EditedJan 6 2022, 9:12 PM

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 marked 11 inline comments as done.Jan 6 2022, 9:15 PM
salman-javed-nz updated this revision to Diff 398087.EditedJan 7 2022, 2:21 AM

Move comments

salman-javed-nz added a comment.EditedJan 17 2022, 2:06 PM

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.

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 added a comment.EditedJan 20 2022, 2:02 AM

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 edited the summary of this revision. (Show Details)

Update according to review (rename NoLintPragmaHandler class to NoLintDirectiveHandler)

salman-javed-nz marked 2 inline comments as done.Jan 20 2022, 2:58 AM

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
162

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 added inline comments.Jan 21 2022, 5:05 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
323

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

323

Tab with spaces

355

Nit: tab with spaces

357

Nit: tab with spaces

359

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

359

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

381

Don't we usually use SourceLocation objects for this?

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 planned changes to this revision.Jan 21 2022, 6:16 PM
salman-javed-nz changed the visibility from "Public (No Login Required)" to "salman-javed-nz (Salman Javed)".

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

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

salman-javed-nz updated this revision to Diff 402302.EditedJan 23 2022, 1:59 AM

Inline short methods into classes

Put implementation-related classes in an anonymous namespace

Fix errors in comments

salman-javed-nz updated this revision to Diff 402305.EditedJan 23 2022, 2:03 AM

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

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

Replace NoLintTokenizer class with a single function, getNoLints()

salman-javed-nz changed the visibility from "salman-javed-nz (Salman Javed)" to "Public (No Login Required)".

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

salman-javed-nz marked 4 inline comments as done.Jan 23 2022, 2:22 AM
salman-javed-nz added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
78

Parameters moved back to shouldSuppressDiagnostic().

salman-javed-nz marked 12 inline comments as done.Jan 23 2022, 2:31 AM
salman-javed-nz added inline comments.
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 marked an inline comment as done.Jan 23 2022, 2:35 AM
salman-javed-nz marked 5 inline comments as done.
salman-javed-nz marked an inline comment as done.Jan 23 2022, 2:42 AM
salman-javed-nz added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
381

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 marked an inline comment as done.Jan 23 2022, 2:42 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
351

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

salman-javed-nz added a comment.EditedJan 23 2022, 4:44 AM

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 added inline comments.Jan 24 2022, 5:21 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
215

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 updated this revision to Diff 402774.EditedJan 25 2022, 12:21 AM

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
salman-javed-nz marked 8 inline comments as done.Jan 25 2022, 12:22 AM
kadircet accepted this revision.Jan 25 2022, 1:42 AM

thanks, lgtm!

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

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.

This revision is now accepted and ready to land.Jan 25 2022, 1:42 AM

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

salman-javed-nz marked an inline comment as done.Jan 25 2022, 3:10 AM
carlosgalvezp accepted this revision.Jan 25 2022, 5:12 AM

Looks great!

This revision was landed with ongoing or failed builds.Jan 26 2022, 3:24 AM
This revision was automatically updated to reflect the committed changes.

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

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.

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/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp