[PATCH] D58547: [clangd] Introduce intermediate representation of formatted text

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This looks pretty nice to me! Sorry for the wait.
Adding @kadircet as hover-guru-in-waiting.



================
Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:814
+                            return Reply(llvm::None);
+                          // FIXME: render as markdown if client supports it.
+                          Hover R;
----------------
(I'd like to see that in this patch if possible, it shouldn't be much work)


================
Comment at: clang-tools-extra/clangd/ClangdServer.h:186
   void findHover(PathRef File, Position Pos,
-                 Callback<llvm::Optional<Hover>> CB);
+                 Callback<llvm::Optional<FormattedString>> CB);
 
----------------
Not sure about switching from `Hover` to `FormattedString` as return type here: LSP hover struct contains a range field (what are the bounds of the hovered thing?) that we may want to populate that doesn't fit in FormattedString.
I'd suggest the function in XRefs.h should return `FormattedString` (and later maybe `pair<FormattedString, Range>`), and the `ClangdServer` version should continue to provide the rendered `Hover`.

(We'll eventually want ClangdServer to return some HoverInfo struct with structured information, as we want embedding clients to be able to render it other ways, and maybe use it to provide extension methods like YCM's `GetType`. But no need to touch that in this patch, we'll end up producing HoverInfo -> FormattedString -> LSP Hover, I guess)


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:41
+
+void FormattedString::appendCodeBlock(std::string Code) {
+  Chunk C;
----------------
you may want to take the language name, and default it to either cpp or nothing.
Including it in the triple-backtick block can trigger syntax highlighting, and editors are probably somewhat likely to actually implement this.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:63
+    case ChunkKind::InlineCodeBlock:
+      R += " `";
+      R += escapeBackticks(C.Contents);
----------------
why do we add surrounding spaces if we don't do the same for text chunks?


================
Comment at: clang-tools-extra/clangd/FormattedString.h:1
+//===--- FormattedString.h ----------------------------------*- C++-*------===//
+//
----------------
This will need tests as you note, which shouldn't be hard.
It's not terribly complicated, but it's the sort of thing that may accumulate special cases.


================
Comment at: clang-tools-extra/clangd/FormattedString.h:27
+  /// Append plain text to the end of the string.
+  void appendText(std::string Text);
+  /// Append a block of C++ code. This translates to a ``` block in markdown.
----------------
I guess this will get an optional parameter to control the style (bold, italic etc)?


================
Comment at: clang-tools-extra/clangd/FormattedString.h:31
+  /// newlines.
+  void appendCodeBlock(std::string Code);
+  /// Append an inline block of C++ code. This translates to the ` block in
----------------
Having various methods that take strings may struggle if we want a lot of composability (e.g. a bullet list whose bullet whose third word is a hyperlink/bold).

(I'm not sure whether this is going to be a real problem, just thinking out loud here)

One alternative extensibility would be to make "containers" methods taking a lambda that renders the body.
e.g.

```
FormattedString S;
S << "std::";
S.bold([&} S << "swap" };
S.list(FormattedString::Numbered, [&] {
  S.item([&] { S << "sometimes you get the wrong overload"; });
  S.item([&] {
    S << "watch out for ";
    S.link("https://en.cppreference.com/w/cpp/language/adl", [&] { S << "ADL"; });
  });
});
S.codeBlock([&] S << "Here is an example"; });
```

Not sure if this is really better, I just have it on my mind because I ended up using it for the `JSON.h` streaming output. We can probably bolt it on later if necessary.


================
Comment at: clang-tools-extra/clangd/FormattedString.h:39
+  std::string renderAsPlainText() const;
+
+private:
----------------
I think we want renderForDebugging() or so which would print the chunks explicitly, a la CodeCompletionString.

This is useful for actual debugging, and for testing methods that return FormattedString (as opposed to the FormattedString->markdown translation)


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:528
+    R.appendText("Declared in ");
+    R.appendText(*NamedScope);
+    R.appendText("\n");
----------------
should this be an inline code block?


================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1157
       EXPECT_NE("", Test.ExpectedHover) << Test.Input;
-      EXPECT_EQ(H->contents.value, Test.ExpectedHover.str()) << Test.Input;
+      EXPECT_EQ(H->renderAsPlainText(), Test.ExpectedHover.str()) << Test.Input;
     } else
----------------
When this lands for real, I think we should assert on the renderForDebugging() output or similar.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58547/new/

https://reviews.llvm.org/D58547