This revision was automatically updated to reflect the committed changes. Closed by commit rGb9d9968f63ab: [clangd] Handle clang-tidy suppression comments for diagnostics inside macro… (authored by nridge).
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75286/new/ https://reviews.llvm.org/D75286 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -272,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()); Index: clang-tools-extra/clangd/ParsedAST.cpp =================================================================== --- clang-tools-extra/clangd/ParsedAST.cpp +++ 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; } } Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h =================================================================== --- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h +++ 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. Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp =================================================================== --- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -296,57 +296,54 @@ return true; } +llvm::Optional<StringRef> 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<StringRef> 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 @@ 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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits