llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Victor Chernyakin (localspook) <details> <summary>Changes</summary> 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`. --- Full diff: https://github.com/llvm/llvm-project/pull/147553.diff 2 Files Affected: - (modified) clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp (+55-60) - (modified) clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h (-2) ``````````diff 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, `````````` </details> https://github.com/llvm/llvm-project/pull/147553 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
