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 @@ -212,15 +212,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 `AvoidIO` is true, 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 AvoidIO = false); /// 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 @@ -123,7 +123,6 @@ : tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory), IsWarningAsError(IsWarningAsError) {} - class ClangTidyContext::CachedGlobList { public: CachedGlobList(StringRef Globs) : Globs(Globs) {} @@ -296,11 +295,38 @@ return true; } +static const char *getCharacterData(const SourceManager &SM, SourceLocation Loc, + bool *Invalid, bool AvoidIO) { + if (!AvoidIO) + return SM.getCharacterData(Loc, Invalid); + + // The rest is similar to the implementation of + // SourceManager::getCharacterData(), but uses ContentCache::getRawBuffer() + // rather than getBuffer() to avoid triggering file I/O if the file contents + // aren't already mapped. + std::pair LocInfo = SM.getDecomposedSpellingLoc(Loc); + bool CharDataInvalid = false; + const SrcMgr::SLocEntry &Entry = + SM.getSLocEntry(LocInfo.first, &CharDataInvalid); + if (CharDataInvalid || !Entry.isFile()) { + *Invalid = true; + return "<<<>>>"; + } + const llvm::MemoryBuffer *Buffer = + Entry.getFile().getContentCache()->getRawBuffer(); + if (!Buffer) { + *Invalid = true; + return "<<<>>>"; + } + return Buffer->getBufferStart() + (CharDataInvalid ? 0 : LocInfo.second); +} + static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc, unsigned DiagID, - const ClangTidyContext &Context) { + const ClangTidyContext &Context, + bool AvoidIO) { bool Invalid; - const char *CharacterData = SM.getCharacterData(Loc, &Invalid); + const char *CharacterData = getCharacterData(SM, Loc, &Invalid, AvoidIO); if (Invalid) return false; @@ -313,8 +339,8 @@ return true; // Check if there's a NOLINTNEXTLINE on the previous line. - const char *BufBegin = - SM.getCharacterData(SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid); + const char *BufBegin = getCharacterData( + SM, SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid, AvoidIO); if (Invalid || P == BufBegin) return false; @@ -344,9 +370,10 @@ static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM, SourceLocation Loc, unsigned DiagID, - const ClangTidyContext &Context) { + const ClangTidyContext &Context, + bool AvoidIO) { while (true) { - if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context)) + if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context, AvoidIO)) return true; if (!Loc.isMacroID()) return false; @@ -360,14 +387,13 @@ bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info, ClangTidyContext &Context, - bool CheckMacroExpansion) { + bool AvoidIO) { 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, AvoidIO); } } // namespace tidy @@ -709,9 +735,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 @@ -285,14 +285,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, + /*AvoidIO=*/true)) { 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) {