From 54d7db165d438bf1bc4f13212a0a7bd3e61aae39 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Wed, 29 Apr 2020 20:10:50 +0200 Subject: [PATCH] [clangd] Move inserted include from detail -> documentation. Summary: Many clients try to display all the detail inline, with poor results. See https://github.com/clangd/clangd/issues/284 Reviewers: hokein Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D79106 --- clang-tools-extra/clangd/CodeComplete.cpp | 37 ++++++++++++++-------- clang-tools-extra/clangd/FormattedString.cpp | 5 +++ clang-tools-extra/clangd/FormattedString.h | 2 ++ .../clangd/unittests/CodeCompleteTests.cpp | 9 +++--- .../clangd/unittests/FormattedStringTests.cpp | 11 +++++++ 5 files changed, 47 insertions(+), 17 deletions(-) diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index abf9e1f..4c7f145 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -1824,6 +1824,21 @@ bool isIndexedForCodeCompletion(const NamedDecl &ND, ASTContext &ASTCtx) { return false; } +// FIXME: find a home for this (that can depend on both markup and Protocol). +static MarkupContent renderDoc(const markup::Document &Doc, MarkupKind Kind) { + MarkupContent Result; + Result.kind = Kind; + switch (Kind) { + case MarkupKind::PlainText: + Result.value.append(Doc.asPlainText()); + break; + case MarkupKind::Markdown: + Result.value.append(Doc.asMarkdown()); + break; + } + return Result; +} + CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const { CompletionItem LSP; const auto *InsertInclude = Includes.empty() ? nullptr : &Includes[0]; @@ -1838,19 +1853,15 @@ CompletionItem CodeCompletion::render(const CodeCompleteOptions &Opts) const { ? std::string(llvm::formatv("[{0} overloads]", BundleSize)) : ReturnType; LSP.deprecated = Deprecated; - if (InsertInclude) - LSP.detail += "\n" + InsertInclude->Header; - if (Documentation) { - LSP.documentation.emplace(); - switch (Opts.DocumentationFormat) { - case MarkupKind::PlainText: - LSP.documentation->value = Documentation->asPlainText(); - break; - case MarkupKind::Markdown: - LSP.documentation->value = Documentation->asMarkdown(); - break; - } - LSP.documentation->kind = Opts.DocumentationFormat; + // Combine header information and documentation in LSP `documentation` field. + // This is not quite right semantically, but tends to display well in editors. + if (InsertInclude || Documentation) { + markup::Document Doc; + if (InsertInclude) + Doc.addParagraph().appendText("From ").appendCode(InsertInclude->Header); + if (Documentation) + Doc.append(*Documentation); + LSP.documentation = renderDoc(Doc, Opts.DocumentationFormat); } LSP.sortText = sortText(Score.Total, Name); LSP.filterText = Name; diff --git a/clang-tools-extra/clangd/FormattedString.cpp b/clang-tools-extra/clangd/FormattedString.cpp index 96a5290..d24103f 100644 --- a/clang-tools-extra/clangd/FormattedString.cpp +++ b/clang-tools-extra/clangd/FormattedString.cpp @@ -434,6 +434,11 @@ Document &Document::operator=(const Document &Other) { return *this; } +void Document::append(Document Other) { + std::move(Other.Children.begin(), Other.Children.end(), + std::back_inserter(Children)); +} + Paragraph &Document::addParagraph() { Children.push_back(std::make_unique()); return *static_cast(Children.back().get()); diff --git a/clang-tools-extra/clangd/FormattedString.h b/clang-tools-extra/clangd/FormattedString.h index ccb758f..4cdaf20 100644 --- a/clang-tools-extra/clangd/FormattedString.h +++ b/clang-tools-extra/clangd/FormattedString.h @@ -88,6 +88,8 @@ public: Document(Document &&) = default; Document &operator=(Document &&) = default; + void append(Document Other); + /// Adds a semantical block that will be separate from others. Paragraph &addParagraph(); /// Inserts a horizontal separator to the document. diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp index c979952..1f30d43 100644 --- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp +++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp @@ -1665,8 +1665,8 @@ TEST(CompletionTest, Render) { EXPECT_EQ(R.insertText, "Foo::x"); EXPECT_EQ(R.insertTextFormat, InsertTextFormat::PlainText); EXPECT_EQ(R.filterText, "x"); - EXPECT_EQ(R.detail, "int\n\"foo.h\""); - EXPECT_EQ(R.documentation->value, "This is x()"); + EXPECT_EQ(R.detail, "int"); + EXPECT_EQ(R.documentation->value, "From \"foo.h\"\nThis is x()"); EXPECT_THAT(R.additionalTextEdits, IsEmpty()); EXPECT_EQ(R.sortText, sortText(1.0, "x")); EXPECT_FALSE(R.deprecated); @@ -1688,7 +1688,8 @@ TEST(CompletionTest, Render) { C.BundleSize = 2; R = C.render(Opts); - EXPECT_EQ(R.detail, "[2 overloads]\n\"foo.h\""); + EXPECT_EQ(R.detail, "[2 overloads]"); + EXPECT_EQ(R.documentation->value, "From \"foo.h\"\nThis is x()"); C.Deprecated = true; R = C.render(Opts); @@ -1696,7 +1697,7 @@ TEST(CompletionTest, Render) { Opts.DocumentationFormat = MarkupKind::Markdown; R = C.render(Opts); - EXPECT_EQ(R.documentation->value, "This is `x()`"); + EXPECT_EQ(R.documentation->value, "From `\"foo.h\"` \nThis is `x()`"); } TEST(CompletionTest, IgnoreRecoveryResults) { diff --git a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp index d3fa1aa..6202f93 100644 --- a/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp +++ b/clang-tools-extra/clangd/unittests/FormattedStringTests.cpp @@ -240,6 +240,17 @@ TEST(Document, Ruler) { EXPECT_EQ(D.asPlainText(), "foo\n\nfoo"); } +TEST(Document, Append) { + Document D; + D.addParagraph().appendText("foo"); + D.addRuler(); + Document E; + E.addRuler(); + E.addParagraph().appendText("bar"); + D.append(std::move(E)); + EXPECT_EQ(D.asMarkdown(), "foo \n\n---\nbar"); +} + TEST(Document, Heading) { Document D; D.addHeading(1).appendText("foo"); -- 2.7.4