llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd Author: Jaagup Averin (JaagupAverin) <details> <summary>Changes</summary> Currently macro expansions are hard capped at 2048. This PR adds the CLI option `--limit-hover-contents` which is passed down in place of the hard coded value. Resolves #<!-- -->153355. --- Full diff: https://github.com/llvm/llvm-project/pull/155105.diff 9 Files Affected: - (modified) clang-tools-extra/clangd/ClangdLSPServer.cpp (+1) - (modified) clang-tools-extra/clangd/ClangdLSPServer.h (+3) - (modified) clang-tools-extra/clangd/ClangdServer.cpp (+4-2) - (modified) clang-tools-extra/clangd/ClangdServer.h (+1-1) - (modified) clang-tools-extra/clangd/Hover.cpp (+5-4) - (modified) clang-tools-extra/clangd/Hover.h (+6-3) - (modified) clang-tools-extra/clangd/tool/Check.cpp (+1-1) - (modified) clang-tools-extra/clangd/tool/ClangdMain.cpp (+10) - (modified) clang-tools-extra/clangd/unittests/HoverTests.cpp (+40) ``````````diff diff --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp b/clang-tools-extra/clangd/ClangdLSPServer.cpp index b445dcf2bbd2e..503935a0a4797 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.cpp +++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp @@ -1268,6 +1268,7 @@ void ClangdLSPServer::onDocumentHighlight( void ClangdLSPServer::onHover(const TextDocumentPositionParams &Params, Callback<std::optional<Hover>> Reply) { Server->findHover(Params.textDocument.uri.file(), Params.position, + Opts.HoverContentsLimit, [Reply = std::move(Reply), this](llvm::Expected<std::optional<HoverInfo>> H) mutable { if (!H) diff --git a/clang-tools-extra/clangd/ClangdLSPServer.h b/clang-tools-extra/clangd/ClangdLSPServer.h index 6ada3fd9e6e47..7c2c44fea1b4a 100644 --- a/clang-tools-extra/clangd/ClangdLSPServer.h +++ b/clang-tools-extra/clangd/ClangdLSPServer.h @@ -64,6 +64,9 @@ class ClangdLSPServer : private ClangdServer::Callbacks, /// Limit the number of references returned (0 means no limit). size_t ReferencesLimit = 0; + /// Limit the number of characters returned when hovering a symbol. + size_t HoverContentsLimit = 0; + /// Flag to hint the experimental modules support is enabled. bool EnableExperimentalModulesSupport = false; }; diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index ac1e9aa5f0ff1..01ec164a56e59 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -843,14 +843,16 @@ void ClangdServer::findDocumentHighlights( } void ClangdServer::findHover(PathRef File, Position Pos, + size_t HoverContentsLimit, Callback<std::optional<HoverInfo>> CB) { - auto Action = [File = File.str(), Pos, CB = std::move(CB), + auto Action = [File = File.str(), Pos, HoverContentsLimit, CB = std::move(CB), this](llvm::Expected<InputsAndAST> InpAST) mutable { if (!InpAST) return CB(InpAST.takeError()); format::FormatStyle Style = getFormatStyleForFile( File, InpAST->Inputs.Contents, *InpAST->Inputs.TFS, false); - CB(clangd::getHover(InpAST->AST, Pos, std::move(Style), Index)); + CB(clangd::getHover(InpAST->AST, Pos, std::move(Style), Index, + HoverContentsLimit)); }; WorkScheduler->runWithAST("Hover", File, std::move(Action), Transient); diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index 4a1eae188f7eb..6a3fa55d1fcc3 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -270,7 +270,7 @@ class ClangdServer { Callback<std::vector<DocumentHighlight>> CB); /// Get code hover for a given position. - void findHover(PathRef File, Position Pos, + void findHover(PathRef File, Position Pos, size_t HoverContentsLimit, Callback<std::optional<HoverInfo>> CB); /// Get information about type hierarchy for a given position. diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp index a7cf45c632827..f352a16c12ecb 100644 --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -755,7 +755,7 @@ HoverInfo evaluateMacroExpansion(unsigned int SpellingBeginOffset, /// Generate a \p Hover object given the macro \p MacroDecl. HoverInfo getHoverContents(const DefinedMacro &Macro, const syntax::Token &Tok, - ParsedAST &AST) { + ParsedAST &AST, size_t HoverContentsLimit) { HoverInfo HI; SourceManager &SM = AST.getSourceManager(); HI.Name = std::string(Macro.Name); @@ -795,7 +795,7 @@ HoverInfo getHoverContents(const DefinedMacro &Macro, const syntax::Token &Tok, for (const auto &ExpandedTok : Expansion->Expanded) { ExpansionText += ExpandedTok.text(SM); ExpansionText += " "; - if (ExpansionText.size() > 2048) { + if (HoverContentsLimit && ExpansionText.size() > HoverContentsLimit) { ExpansionText.clear(); break; } @@ -1250,7 +1250,8 @@ void maybeAddUsedSymbols(ParsedAST &AST, HoverInfo &HI, const Inclusion &Inc) { std::optional<HoverInfo> getHover(ParsedAST &AST, Position Pos, const format::FormatStyle &Style, - const SymbolIndex *Index) { + const SymbolIndex *Index, + size_t HoverContentsLimit) { static constexpr trace::Metric HoverCountMetric( "hover", trace::Metric::Counter, "case"); PrintingPolicy PP = @@ -1297,7 +1298,7 @@ std::optional<HoverInfo> getHover(ParsedAST &AST, Position Pos, HighlightRange = Tok.range(SM).toCharRange(SM); if (auto M = locateMacroAt(Tok, AST.getPreprocessor())) { HoverCountMetric.record(1, "macro"); - HI = getHoverContents(*M, Tok, AST); + HI = getHoverContents(*M, Tok, AST, HoverContentsLimit); if (auto DefLoc = M->Info->getDefinitionLoc(); DefLoc.isValid()) { include_cleaner::Macro IncludeCleanerMacro{ AST.getPreprocessor().getIdentifierInfo(Tok.text(SM)), DefLoc}; diff --git a/clang-tools-extra/clangd/Hover.h b/clang-tools-extra/clangd/Hover.h index 614180a7b9846..4dbd7c050421d 100644 --- a/clang-tools-extra/clangd/Hover.h +++ b/clang-tools-extra/clangd/Hover.h @@ -163,10 +163,13 @@ inline bool operator==(const HoverInfo::Param &LHS, std::tie(RHS.Type, RHS.Name, RHS.Default); } +constexpr size_t DefaultHoverContentsLimit = 2048; + /// Get the hover information when hovering at \p Pos. -std::optional<HoverInfo> getHover(ParsedAST &AST, Position Pos, - const format::FormatStyle &Style, - const SymbolIndex *Index); +std::optional<HoverInfo> +getHover(ParsedAST &AST, Position Pos, const format::FormatStyle &Style, + const SymbolIndex *Index, + size_t HoverContentsLimit = DefaultHoverContentsLimit); } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp index df8d075e80596..d103ef3f5aed3 100644 --- a/clang-tools-extra/clangd/tool/Check.cpp +++ b/clang-tools-extra/clangd/tool/Check.cpp @@ -441,7 +441,7 @@ class Checker { unsigned Definitions = locateSymbolAt(*AST, Pos, &Index).size(); vlog(" definition: {0}", Definitions); - auto Hover = getHover(*AST, Pos, Style, &Index); + auto Hover = getHover(*AST, Pos, Style, &Index, Opts.HoverContentsLimit); vlog(" hover: {0}", Hover.has_value()); unsigned DocHighlights = findDocumentHighlights(*AST, Pos).size(); diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index f287439f10cab..5e8755d643d7f 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -328,6 +328,15 @@ opt<int> ReferencesLimit{ init(1000), }; +opt<int> HoverContentsLimit{ + "limit-hover-contents", + cat(Features), + desc("Limit the number of characters returned when hovering a symbol. " + "Texts longer than this value will be dropped (not truncated). " + "0 means no limit (default=2048)"), + init(DefaultHoverContentsLimit), +}; + opt<int> RenameFileLimit{ "rename-file-limit", cat(Features), @@ -925,6 +934,7 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var Opts.BackgroundIndex = EnableBackgroundIndex; Opts.BackgroundIndexPriority = BackgroundIndexPriority; Opts.ReferencesLimit = ReferencesLimit; + Opts.HoverContentsLimit = HoverContentsLimit; Opts.Rename.LimitFiles = RenameFileLimit; auto PAI = createProjectAwareIndex( [SupportContainedRefs = Opts.EnableOutgoingCalls]( diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp index 6407349d9af9b..fe0f4334746ca 100644 --- a/clang-tools-extra/clangd/unittests/HoverTests.cpp +++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp @@ -4461,6 +4461,46 @@ constexpr u64 pow_with_mod(u64 a, u64 b, u64 p) { EXPECT_TRUE(H->Type); } +TEST(Hover, HoverContentsLimit) { + const char *const Code = + R"cpp( + #define C(A) A##A // Concatenate + #define E(A) C(A) // Expand + #define Z0032 00000000000000000000000000000000 + #define Z0064 E(Z0032) + #define Z0128 E(Z0064) + #define Z0256 E(Z0128) + #define Z0512 E(Z0256) + #define Z1024 E(Z0512) + #define Z2048 E(Z1024) + #define Z4096 E(Z2048) // 4096 zeroes + int main() { return [[^Z4096]]; } + )cpp"; + + struct { + size_t HoverContentsLimit; + const std::string ExpectedDefinition; + } Cases[] = { + // With a limit of 2048, the macro expansion should get dropped. + {2048, "#define Z4096 E(Z2048)"}, + // With a limit of 8192, the macro expansion should be fully expanded. + {8192, std::string("#define Z4096 E(Z2048)\n\n") + + std::string("// Expands to\n") + std::string(4096, '0')}, + }; + for (const auto &Case : Cases) { + SCOPED_TRACE(Code); + + Annotations T(Code); + TestTU TU = TestTU::withCode(T.code()); + auto AST = TU.build(); + auto H = getHover(AST, T.point(), format::getLLVMStyle(), nullptr, + Case.HoverContentsLimit); + ASSERT_TRUE(H); + + EXPECT_EQ(H->Definition, Case.ExpectedDefinition); + } +}; + TEST(Hover, FunctionParameters) { struct { const char *const Code; `````````` </details> https://github.com/llvm/llvm-project/pull/155105 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
