https://github.com/localspook created
https://github.com/llvm/llvm-project/pull/147553
I plan to do more work in this area, but I feel this is a good first set of
changes.
Summary:
- `NoLintBlockToken` is too big: it stores a whole `NoLintToken` inside itself,
when all it needs from that `NoLintToken` is its `Pos` and `ChecksGlob`.
- `formNoLintBlocks` builds up a vector of unmatched tokens, which are later
transformed into errors. We can skip the middle step and make
`formNoLintBlocks` create errors directly.
- In `generateCache`, the line `Cache[FileName] = ...;` default-constructs a
cache entry only to immediately overwrite it. We can avoid that by using
`Cache.try_emplace(FileName, ...);` instead.
- `NoLintToken`'s constructor takes `const std::optional<std::string>&` when
all it needs is `const std::optional<StringRef>&`. This forces its caller,
`getNoLints`, to create temporary strings.
- `NoLintToken::checks` returns `std::optional<std::string>` by value, creating
unnecessary copies in `formNoLintBlocks`.
>From 9b772adf9cf32f0ae47011e41c16d1dc180887cd Mon Sep 17 00:00:00 2001
From: Victor Chernyakin <chernyakin.victo...@outlook.com>
Date: Mon, 7 Jul 2025 18:00:28 -0700
Subject: [PATCH] [clang-tidy][NFC] Do less unnecessary work in
`NoLintDirectiveHandler`
---
.../clang-tidy/NoLintDirectiveHandler.cpp | 115 +++++++++---------
.../clang-tidy/NoLintDirectiveHandler.h | 2 -
2 files changed, 55 insertions(+), 62 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
index ed03b456f4954..bbae2c171f790 100644
--- a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -25,10 +25,9 @@
#include "llvm/ADT/StringSwitch.h"
#include <cassert>
#include <cstddef>
-#include <iterator>
+#include <memory>
#include <optional>
#include <string>
-#include <tuple>
#include <utility>
namespace clang::tidy {
@@ -79,7 +78,7 @@ class NoLintToken {
// - 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 std::optional<std::string> &Checks)
+ const std::optional<StringRef> &Checks)
: Type(Type), Pos(Pos), ChecksGlob(std::make_unique<CachedGlobList>(
Checks.value_or("*"),
/*KeepNegativeGlobs=*/false)) {
@@ -93,15 +92,17 @@ class NoLintToken {
// The location of the first character, "N", in "NOLINT".
size_t Pos;
+ // A glob of the checks this NOLINT token disables.
+ std::unique_ptr<CachedGlobList> ChecksGlob;
+
// If this NOLINT specifies checks, return the checks.
- std::optional<std::string> checks() const { return Checks; }
+ const std::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:
std::optional<std::string> Checks;
- std::unique_ptr<CachedGlobList> ChecksGlob;
};
} // namespace
@@ -131,11 +132,11 @@ static SmallVector<NoLintToken> getNoLints(StringRef
Buffer) {
continue;
// Get checks, if specified.
- std::optional<std::string> Checks;
+ std::optional<StringRef> 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();
+ Checks = Buffer.slice(Pos, ClosingBracket);
Pos = ClosingBracket + 1;
}
}
@@ -155,34 +156,51 @@ 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());
- }
+ NoLintBlockToken(size_t BeginPos, size_t EndPos,
+ std::unique_ptr<CachedGlobList> ChecksGlob)
+ : BeginPos(BeginPos), EndPos(EndPos), ChecksGlob(std::move(ChecksGlob))
{}
// 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);
+ return (BeginPos < DiagPos) && (DiagPos < EndPos) &&
+ ChecksGlob->contains(DiagName);
}
private:
- NoLintToken Begin;
+ size_t BeginPos;
size_t EndPos;
+ std::unique_ptr<CachedGlobList> ChecksGlob;
};
} // namespace
+// 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;
+}
+
// 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`.
+// `NoLintBlockToken`s. If any BEGINs or ENDs are left over, a diagnostic is
+// written to `NoLintErrors`.
static SmallVector<NoLintBlockToken>
-formNoLintBlocks(SmallVector<NoLintToken> NoLints,
- SmallVectorImpl<NoLintToken> &UnmatchedTokens) {
+formNoLintBlocks(SmallVector<NoLintToken> NoLints, const SourceManager &SrcMgr,
+ FileID File,
+ SmallVectorImpl<tooling::Diagnostic> &NoLintErrors) {
SmallVector<NoLintBlockToken> CompletedBlocks;
SmallVector<NoLintToken> Stack;
@@ -196,16 +214,20 @@ formNoLintBlocks(SmallVector<NoLintToken> NoLints,
// 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())
+ if (!Stack.empty() && Stack.back().checks() == NoLint.checks()) {
// The previous block is being closed. Pop one element off the stack.
- CompletedBlocks.emplace_back(Stack.pop_back_val(), NoLint);
- else
+ CompletedBlocks.emplace_back(Stack.back().Pos, NoLint.Pos,
+ std::move(Stack.back().ChecksGlob));
+ Stack.pop_back();
+ } else
// Trying to close the wrong block.
- UnmatchedTokens.emplace_back(std::move(NoLint));
+ NoLintErrors.emplace_back(makeNoLintError(SrcMgr, File, NoLint));
}
}
- llvm::move(Stack, std::back_inserter(UnmatchedTokens));
+ for (const NoLintToken &NoLint : Stack)
+ NoLintErrors.emplace_back(makeNoLintError(SrcMgr, File, NoLint));
+
return CompletedBlocks;
}
@@ -274,7 +296,7 @@ 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);
+ return {StartPos, EndPos};
}
// Whether the line has a NOLINT of type = `Type` that can suppress the
@@ -317,9 +339,7 @@ bool NoLintDirectiveHandler::Impl::diagHasNoLint(
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);
+ const auto [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>.
@@ -349,46 +369,21 @@ bool NoLintDirectiveHandler::Impl::diagHasNoLint(
return false;
// Do we have cached NOLINT block locations for this file?
- if (Cache.count(*FileName) == 0)
+ if (!Cache.contains(*FileName))
// 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(std::move(NoLints), UnmatchedTokens);
-
- // Raise error for any BEGIN/END left over.
- for (const NoLintToken &NoLint : UnmatchedTokens)
- NoLintErrors.emplace_back(makeNoLintError(SrcMgr, File, NoLint));
+ // Read entire file to get all NOLINTs and match each BEGIN with its
+ // corresponding END, raising errors for any BEGIN or END that is unmatched.
+ Cache.try_emplace(FileName, formNoLintBlocks(getNoLints(Buffer), SrcMgr,
File,
+ NoLintErrors));
}
//===----------------------------------------------------------------------===//
diff --git a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
index f66285672d04a..e862195abaabb 100644
--- a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
+++ b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h
@@ -31,8 +31,6 @@ class NoLintDirectiveHandler {
public:
NoLintDirectiveHandler();
~NoLintDirectiveHandler();
- NoLintDirectiveHandler(const NoLintDirectiveHandler &) = delete;
- NoLintDirectiveHandler &operator=(const NoLintDirectiveHandler &) = delete;
bool shouldSuppress(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Diag, llvm::StringRef DiagName,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
