diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -216,15 +216,12 @@ /// This does not handle suppression of notes following a suppressed diagnostic; /// that is left to the caller is it requires maintaining state in between calls /// to this function. -/// The `CheckMacroExpansion` parameter determines whether the function should -/// handle the case where the diagnostic is inside a macro expansion. A degree -/// of control over this is needed because handling this case can require -/// examining source files other than the one in which the diagnostic is -/// located, and in some use cases we cannot rely on such other files being -/// mapped in the SourceMapper. +/// 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. bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info, ClangTidyContext &Context, - bool CheckMacroExpansion = true); + bool AllowIO = true); /// A diagnostic consumer that turns each \c Diagnostic into a /// \c SourceManager-independent \c ClangTidyError. diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -124,7 +124,6 @@ : tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory), IsWarningAsError(IsWarningAsError) {} - class ClangTidyContext::CachedGlobList { public: CachedGlobList(StringRef Globs) : Globs(Globs) {} @@ -297,57 +296,54 @@ return true; } +llvm::Optional getBuffer(const SourceManager &SM, FileID File, + bool AllowIO) { + // This is similar to the implementation of SourceManager::getCharacterData(), + // but uses ContentCache::getRawBuffer() rather than getBuffer() if + // AllowIO=false, to avoid triggering file I/O if the file contents aren't + // already mapped. + bool CharDataInvalid = false; + const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(File, &CharDataInvalid); + if (CharDataInvalid || !Entry.isFile()) + return llvm::None; + const SrcMgr::ContentCache *Cache = Entry.getFile().getContentCache(); + const llvm::MemoryBuffer *Buffer = + AllowIO ? Cache->getBuffer(SM.getDiagnostics(), SM.getFileManager(), + SourceLocation(), &CharDataInvalid) + : Cache->getRawBuffer(); + if (!Buffer || CharDataInvalid) + return llvm::None; + return Buffer->getBuffer(); +} + static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc, unsigned DiagID, - const ClangTidyContext &Context) { - bool Invalid; - const char *CharacterData = SM.getCharacterData(Loc, &Invalid); - if (Invalid) + const ClangTidyContext &Context, + bool AllowIO) { + FileID File; + unsigned Offset; + std::tie(File, Offset) = SM.getDecomposedSpellingLoc(Loc); + llvm::Optional Buffer = getBuffer(SM, File, AllowIO); + if (!Buffer) return false; // Check if there's a NOLINT on this line. - const char *P = CharacterData; - while (*P != '\0' && *P != '\r' && *P != '\n') - ++P; - StringRef RestOfLine(CharacterData, P - CharacterData + 1); + StringRef RestOfLine = Buffer->substr(Offset).split('\n').first; if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context)) return true; // Check if there's a NOLINTNEXTLINE on the previous line. - const char *BufBegin = - SM.getCharacterData(SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid); - if (Invalid || P == BufBegin) - return false; - - // Scan backwards over the current line. - P = CharacterData; - while (P != BufBegin && *P != '\n') - --P; - - // If we reached the begin of the file there is no line before it. - if (P == BufBegin) - return false; - - // Skip over the newline. - --P; - const char *LineEnd = P; - - // Now we're on the previous line. Skip to the beginning of it. - while (P != BufBegin && *P != '\n') - --P; - - RestOfLine = StringRef(P, LineEnd - P + 1); - if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context)) - return true; - - return false; + StringRef PrevLine = + Buffer->substr(0, Offset).rsplit('\n').first.rsplit('\n').second; + return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context); } static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM, SourceLocation Loc, unsigned DiagID, - const ClangTidyContext &Context) { + const ClangTidyContext &Context, + bool AllowIO) { while (true) { - if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context)) + if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context, AllowIO)) return true; if (!Loc.isMacroID()) return false; @@ -361,14 +357,13 @@ bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info, ClangTidyContext &Context, - bool CheckMacroExpansion) { + bool AllowIO) { return Info.getLocation().isValid() && DiagLevel != DiagnosticsEngine::Error && DiagLevel != DiagnosticsEngine::Fatal && - (CheckMacroExpansion ? LineIsMarkedWithNOLINTinMacro - : LineIsMarkedWithNOLINT)(Info.getSourceManager(), - Info.getLocation(), - Info.getID(), Context); + LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(), + Info.getLocation(), Info.getID(), + Context, AllowIO); } } // namespace tidy @@ -710,9 +705,9 @@ const tooling::DiagnosticMessage &M1 = LHS.Message; const tooling::DiagnosticMessage &M2 = RHS.Message; - return - std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) < - std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message); + return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, + M1.Message) < + std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message); } }; struct EqualClangTidyError { diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -310,14 +310,13 @@ // Check for suppression comment. Skip the check for diagnostics not // in the main file, because we don't want that function to query the // source buffer for preamble files. For the same reason, we ask - // shouldSuppressDiagnostic not to follow macro expansions, since - // those might take us into a preamble file as well. + // shouldSuppressDiagnostic to avoid I/O. bool IsInsideMainFile = Info.hasSourceManager() && isInsideMainFile(Info.getLocation(), Info.getSourceManager()); - if (IsInsideMainFile && tidy::shouldSuppressDiagnostic( - DiagLevel, Info, *CTContext, - /* CheckMacroExpansion = */ false)) { + if (IsInsideMainFile && + tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext, + /*AllowIO=*/false)) { return DiagnosticsEngine::Ignored; } } diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -61,9 +61,7 @@ arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement; } -MATCHER_P(FixMessage, Message, "") { - return arg.Message == Message; -} +MATCHER_P(FixMessage, Message, "") { return arg.Message == Message; } MATCHER_P(EqualToLSPDiag, LSPDiag, "LSP diagnostic " + llvm::to_string(LSPDiag)) { @@ -258,13 +256,13 @@ Diag(Test.range("macroarg"), "multiple unsequenced modifications to 'y'"), AllOf( - Diag(Test.range("main"), - "use a trailing return type for this function"), - DiagSource(Diag::ClangTidy), - DiagName("modernize-use-trailing-return-type"), - // Verify that we don't have "[check-name]" suffix in the message. - WithFix(FixMessage("use a trailing return type for this function"))) - )); + Diag(Test.range("main"), + "use a trailing return type for this function"), + DiagSource(Diag::ClangTidy), + DiagName("modernize-use-trailing-return-type"), + // Verify that we don't have "[check-name]" suffix in the message. + WithFix(FixMessage( + "use a trailing return type for this function"))))); } TEST(DiagnosticTest, ClangTidySuppressionComment) { @@ -274,7 +272,11 @@ double d = 8 / i; // NOLINT // NOLINTNEXTLINE double e = 8 / i; - double f = [[8]] / i; + #define BAD 8 / i + double f = BAD; // NOLINT + double g = [[8]] / i; + #define BAD2 BAD + double h = BAD2; // NOLINT } )cpp"); TestTU TU = TestTU::withCode(Main.code()); @@ -836,8 +838,9 @@ auto Index = buildIndexWithSymbol({}); TU.ExternalIndex = Index.get(); - EXPECT_THAT(TU.build().getDiagnostics(), - ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'"))); + EXPECT_THAT( + TU.build().getDiagnostics(), + ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'"))); } TEST(DiagsInHeaders, DiagInsideHeader) {