From b9d9968f63ab8f24b300c69be11eadda3d405ac5 Mon Sep 17 00:00:00 2001 From: Nathan Ridge Date: Sun, 29 Mar 2020 15:13:13 -0400 Subject: [PATCH] [clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions Summary: Not handling this was a side-effect of being overly cautious when trying to avoid reading files for which clangd doesn't have the source mapped. Fixes https://github.com/clangd/clangd/issues/266 Reviewers: sammccall Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D75286 --- .../ClangTidyDiagnosticConsumer.cpp | 80 +++++++++---------- .../clang-tidy/ClangTidyDiagnosticConsumer.h | 11 +-- clang-tools-extra/clangd/ParsedAST.cpp | 9 +-- .../clangd/unittests/DiagnosticsTests.cpp | 6 +- 4 files changed, 51 insertions(+), 55 deletions(-) diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp index 10d5bdb3b3af..d30552734679 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -296,57 +296,54 @@ static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line, return true; } +llvm::Optional getBuffer(const SourceManager &SM, FileID File, + bool AllowIO) { + // This is similar to the implementation of SourceManager::getBufferData(), + // 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; @@ -360,14 +357,13 @@ namespace tidy { 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 diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h index 18ce478805f8..2bb3296b150d 100644 --- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h @@ -216,15 +216,12 @@ private: /// 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/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 91c71789dbe0..1d6997f0b4d4 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -310,14 +310,13 @@ ParsedAST::build(llvm::StringRef Version, // 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 index d72f722921a8..00dd1f864813 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -272,7 +272,11 @@ TEST(DiagnosticTest, ClangTidySuppressionComment) { 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());