From 19eaad94c47f92dd23989b0b6832bb6751dde979 Mon Sep 17 00:00:00 2001 From: Salman Javed Date: Wed, 26 Jan 2022 23:56:27 +1300 Subject: [PATCH] [clang-tidy] Cache the locations of NOLINTBEGIN/END blocks Support for NOLINT(BEGIN/END) blocks (implemented in D108560) is currently costly. This patch aims to improve the performance with the following changes: - The use of tokenized NOLINTs instead of a series 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`. Differential Revision: https://reviews.llvm.org/D116085 --- clang-tools-extra/clang-tidy/CMakeLists.txt | 1 + .../ClangTidyDiagnosticConsumer.cpp | 230 +--------- .../clang-tidy/ClangTidyDiagnosticConsumer.h | 47 +- .../clang-tidy/NoLintDirectiveHandler.cpp | 415 ++++++++++++++++++ .../clang-tidy/NoLintDirectiveHandler.h | 51 +++ clang-tools-extra/clangd/ParsedAST.cpp | 12 +- .../nolintbeginend/1st-translation-unit.cpp | 5 + .../nolintbeginend/2nd-translation-unit.cpp | 6 + .../test/clang-tidy/infrastructure/nolint.cpp | 51 ++- .../infrastructure/nolintbeginend-LIFO.cpp | 19 + .../nolintbeginend-begin-all-end-glob.cpp | 16 + .../nolintbeginend-begin-all-end-specific.cpp | 16 + .../nolintbeginend-begin-glob-end-all.cpp | 16 + ...nolintbeginend-begin-glob-end-specific.cpp | 16 + ...lintbeginend-begin-global-end-specific.cpp | 25 -- ...lintbeginend-begin-multiple-end-single.cpp | 22 + ...lintbeginend-begin-single-end-multiple.cpp | 22 + .../nolintbeginend-begin-specific-end-all.cpp | 16 + ...nolintbeginend-begin-specific-end-glob.cpp | 16 + ...lintbeginend-begin-specific-end-global.cpp | 25 -- .../nolintbeginend-mismatched-check-names.cpp | 21 - .../nolintbeginend-mismatched-delims.cpp | 1 - .../nolintbeginend-multiple-TUs.cpp | 6 + .../nolintbeginend-typo-in-check-name.cpp | 3 + .../infrastructure/nolintbeginend.cpp | 28 +- 25 files changed, 752 insertions(+), 334 deletions(-) create mode 100644 clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp create mode 100644 clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp delete mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp delete mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.cpp create mode 100644 clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-multiple-TUs.cpp diff --git a/clang-tools-extra/clang-tidy/CMakeLists.txt b/clang-tools-extra/clang-tidy/CMakeLists.txt index 075e9f9909d65..8a953eeea2759 100644 --- a/clang-tools-extra/clang-tidy/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/CMakeLists.txt @@ -17,6 +17,7 @@ add_clang_library(clangTidy ClangTidyProfiling.cpp ExpandModularHeadersPPCallbacks.cpp GlobList.cpp + NoLintDirectiveHandler.cpp DEPENDS ClangSACheckers diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 66f60ec60b605..04721a2d3a020 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -18,6 +18,7 @@ #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" @@ -188,7 +189,7 @@ DiagnosticBuilder ClangTidyContext::diag( return DiagEngine->Report(ID); } -DiagnosticBuilder ClangTidyContext::diag(const ClangTidyError &Error) { +DiagnosticBuilder ClangTidyContext::diag(const tooling::Diagnostic &Error) { SourceManager &SM = DiagEngine->getSourceManager(); llvm::ErrorOr File = SM.getFileManager().getFile(Error.Message.FilePath); @@ -206,6 +207,15 @@ DiagnosticBuilder ClangTidyContext::configurationDiag( return diag("clang-tidy-config", Message, Level); } +bool ClangTidyContext::shouldSuppressDiagnostic( + DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info, + SmallVectorImpl &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); } @@ -307,218 +317,9 @@ void ClangTidyDiagnosticConsumer::finalizeLastError() { 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 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 tallyNolintBegins( - const ClangTidyContext &Context, const SourceManager &SM, - StringRef CheckName, SmallVector Lines, SourceLocation LinesLoc, - SmallVector> &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 &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> NolintBegins; - - // Check if there's an open NOLINT(BEGIN...END) block on the previous lines. - SmallVector 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 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 &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 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 &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 &SuppressionErrors, bool AllowIO, - bool EnableNolintBlocks) { - return Info.getLocation().isValid() && - DiagLevel != DiagnosticsEngine::Error && - DiagLevel != DiagnosticsEngine::Fatal && - lineIsMarkedWithNOLINTinMacro(Info, Context, SuppressionErrors, - AllowIO, EnableNolintBlocks); -} - const llvm::StringMap * getFixIt(const tooling::Diagnostic &Diagnostic, bool GetFixFromNotes) { if (!Diagnostic.Message.Fix.empty()) @@ -545,12 +346,15 @@ void ClangTidyDiagnosticConsumer::HandleDiagnostic( if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note) return; - SmallVector SuppressionErrors; - if (shouldSuppressDiagnostic(DiagLevel, Info, Context, SuppressionErrors, - EnableNolintBlocks)) { + SmallVector 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; } diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h index 69f1ceddd9bd2..e41003262cccf 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -11,6 +11,7 @@ #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" @@ -95,13 +96,33 @@ class ClangTidyContext { 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 &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. @@ -208,29 +229,9 @@ class ClangTidyContext { 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 &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 diff --git a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp new file mode 100644 index 0000000000000..6723b752fc848 --- /dev/null +++ b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp @@ -0,0 +1,415 @@ +//===-- 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 +#include +#include +#include +#include +#include +#include + +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 strToNoLintType(StringRef Str) { + auto Type = llvm::StringSwitch>(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 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 &Checks) + : Type(Type), Pos(Pos), ChecksGlob(std::make_unique( + 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 checks() const { return Checks; } + + // Whether this NOLINT applies to the provided check. + bool suppresses(StringRef Check) const { return ChecksGlob->contains(Check); } + +private: + Optional Checks; + std::unique_ptr ChecksGlob; +}; + +} // namespace + +// Consume the entire buffer and return all `NoLintToken`s that were found. +static SmallVector getNoLints(StringRef Buffer) { + static constexpr llvm::StringLiteral NOLINT = "NOLINT"; + SmallVector 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 = + strToNoLintType(Buffer.slice(NoLintPos, Pos)); + if (!NoLintType) + continue; + + // Get checks, if specified. + Optional 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 +formNoLintBlocks(SmallVector NoLints, + SmallVectorImpl &UnmatchedTokens) { + SmallVector CompletedBlocks; + SmallVector 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 &NoLintErrors, + bool AllowIO, bool EnableNoLintBlocks); + +private: + bool diagHasNoLintInMacro(const Diagnostic &Diag, StringRef DiagName, + SmallVectorImpl &NoLintErrors, + bool AllowIO, bool EnableNoLintBlocks); + + bool diagHasNoLint(StringRef DiagName, SourceLocation DiagLoc, + const SourceManager &SrcMgr, + SmallVectorImpl &NoLintErrors, + bool AllowIO, bool EnableNoLintBlocks); + + void generateCache(const SourceManager &SrcMgr, StringRef FileName, + FileID File, StringRef Buffer, + SmallVectorImpl &NoLintErrors); + + llvm::StringMap> Cache; +}; + +bool NoLintDirectiveHandler::Impl::shouldSuppress( + DiagnosticsEngine::Level DiagLevel, const Diagnostic &Diag, + StringRef DiagName, SmallVectorImpl &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 &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 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 LineStartAndEnd, + NoLintType Type, StringRef DiagName) { + // Get all NOLINTs on the line. + Buffer = Buffer.slice(LineStartAndEnd.first, LineStartAndEnd.second); + SmallVector 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 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 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 &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 . + Optional FileName = SrcMgr.getNonBuiltinFilenameForID(File); + if (!FileName) + return false; + + // Get file contents. + Optional 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 &NoLintErrors) { + // Read entire file to get all NOLINTs. + SmallVector NoLints = getNoLints(Buffer); + + // Match each BEGIN with its corresponding END. + SmallVector 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)); +} + +//===----------------------------------------------------------------------===// +// NoLintDirectiveHandler +//===----------------------------------------------------------------------===// + +NoLintDirectiveHandler::NoLintDirectiveHandler() + : PImpl(std::make_unique()) {} + +NoLintDirectiveHandler::~NoLintDirectiveHandler() = default; + +bool NoLintDirectiveHandler::shouldSuppress( + DiagnosticsEngine::Level DiagLevel, const Diagnostic &Diag, + StringRef DiagName, SmallVectorImpl &NoLintErrors, + bool AllowIO, bool EnableNoLintBlocks) { + return PImpl->shouldSuppress(DiagLevel, Diag, DiagName, NoLintErrors, AllowIO, + EnableNoLintBlocks); +} + +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h new file mode 100644 index 0000000000000..db9fd593283c7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/NoLintDirectiveHandler.h @@ -0,0 +1,51 @@ +//===-- 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 + +namespace clang { +namespace tooling { +struct Diagnostic; +} // namespace tooling +} // namespace clang + +namespace llvm { +template 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 &NoLintErrors, + bool AllowIO, bool EnableNoLintBlocks); + +private: + class Impl; + std::unique_ptr PImpl; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_NOLINTDIRECTIVEHANDLER_H diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 9c64c645130bf..86c4b2151243c 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -10,6 +10,7 @@ #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" @@ -468,12 +469,11 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, bool IsInsideMainFile = Info.hasSourceManager() && isInsideMainFile(Info.getLocation(), Info.getSourceManager()); - SmallVector TidySuppressedErrors; - if (IsInsideMainFile && - tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext, - TidySuppressedErrors, - /*AllowIO=*/false, - /*EnableNolintBlocks=*/false)) { + SmallVector TidySuppressedErrors; + if (IsInsideMainFile && CTContext->shouldSuppressDiagnostic( + DiagLevel, Info, TidySuppressedErrors, + /*AllowIO=*/false, + /*EnableNolintBlocks=*/false)) { // FIXME: should we expose the suppression error (invalid use of // NOLINT comments)? return DiagnosticsEngine::Ignored; diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp new file mode 100644 index 0000000000000..d90fc069c9789 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/1st-translation-unit.cpp @@ -0,0 +1,5 @@ +// NOLINTBEGIN +class A { A(int i); }; +// NOLINTEND + +class B { B(int i); }; diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp new file mode 100644 index 0000000000000..0497037d31b82 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/nolintbeginend/2nd-translation-unit.cpp @@ -0,0 +1,6 @@ + +class A { A(int i); }; + +// NOLINTBEGIN +class B { B(int i); }; +// NOLINTEND diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp index 4b80bbe4eb3ca..9ef194232b6b7 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp @@ -28,11 +28,56 @@ 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 +class C17 { C17(int i); }; // NOLINT(google-explicit-constructor): suppress check because + +// 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] @@ -71,4 +116,4 @@ int array2[10]; // NOLINT(cppcoreguidelines-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) diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp new file mode 100644 index 0000000000000..ee5b1cca755ab --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-LIFO.cpp @@ -0,0 +1,19 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp new file mode 100644 index 0000000000000..90b9fa9883024 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-glob.cpp @@ -0,0 +1,16 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp new file mode 100644 index 0000000000000..6ffa914e4ef0b --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-all-end-specific.cpp @@ -0,0 +1,16 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp new file mode 100644 index 0000000000000..3697d5c11e2e2 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-all.cpp @@ -0,0 +1,16 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp new file mode 100644 index 0000000000000..5bdb117f20242 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-glob-end-specific.cpp @@ -0,0 +1,16 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp deleted file mode 100644 index 01e69ddf6352a..0000000000000 --- a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-global-end-specific.cpp +++ /dev/null @@ -1,25 +0,0 @@ -// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp new file mode 100644 index 0000000000000..150913ce0ec69 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-multiple-end-single.cpp @@ -0,0 +1,22 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp new file mode 100644 index 0000000000000..f9a915c890cbb --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-single-end-multiple.cpp @@ -0,0 +1,22 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp new file mode 100644 index 0000000000000..decfe2dd5a4c1 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-all.cpp @@ -0,0 +1,16 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp new file mode 100644 index 0000000000000..a9f904ccce138 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-glob.cpp @@ -0,0 +1,16 @@ +// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.cpp deleted file mode 100644 index c6fcfddd09ba8..0000000000000 --- a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-begin-specific-end-global.cpp +++ /dev/null @@ -1,25 +0,0 @@ -// 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-check-names.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-check-names.cpp index 957b0b341a3cc..8d7786fb8c712 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-check-names.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-check-names.cpp @@ -16,24 +16,3 @@ auto Num = (unsigned int)(-1); // 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp index 7ed5fb820e509..4b8947e369f92 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-mismatched-delims.cpp @@ -11,4 +11,3 @@ class A { A(int i); }; // 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 diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-multiple-TUs.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-multiple-TUs.cpp new file mode 100644 index 0000000000000..773e023cb22c9 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-multiple-TUs.cpp @@ -0,0 +1,6 @@ +// 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 diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp index 0f2f9994c1fa0..57e1ff331c8ba 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend-typo-in-check-name.cpp @@ -11,3 +11,6 @@ class A { A(int i); }; // 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] diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp index 8379128530180..ae97bc5b51441 100644 --- a/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp +++ b/clang-tools-extra/test/clang-tidy/infrastructure/nolintbeginend.cpp @@ -58,30 +58,24 @@ 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); }; @@ -114,28 +108,28 @@ MACRO_WRAPPED_WITH_NO_LINT 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]; @@ -154,4 +148,4 @@ int array3[10]; int array4[10]; // NOLINTEND(*-avoid-c-arrays) -// CHECK-MESSAGES: Suppressed 27 warnings (27 NOLINT). +// CHECK-MESSAGES: Suppressed 26 warnings (26 NOLINT).