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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:41
+
+void FormattedString::appendCodeBlock(std::string Code) {
+  Chunk C;
----------------
sammccall wrote:
> 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.
Done. Defaulting to 'cpp'.
VSCode actually does syntax highlighting there and it looks nice.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:63
+    case ChunkKind::InlineCodeBlock:
+      R += " `";
+      R += escapeBackticks(C.Contents);
----------------
sammccall wrote:
> why do we add surrounding spaces if we don't do the same for text chunks?
a leftover from my first prototype.
They don't seem to be necessary, removed them.


================
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
----------------
sammccall wrote:
> 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.
Agree that the current approach won't generalize to more complicated cases without some design work.
The lambda-heavy code is hard to read to my personal taste, but may actually be a good option in practice.
I'd also try playing around with some lambda-free alternatives to see if they lead to better syntax without compromising extensibility or making the implementation overly complex.

I also think we should bolt on this until we hit more use-cases with more extensive needs for formatting markdown. 


================
Comment at: clang-tools-extra/clangd/FormattedString.h:39
+  std::string renderAsPlainText() const;
+
+private:
----------------
sammccall wrote:
> 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)
That would be a useful addition. I've added a FIXME for now and testing plaintext in XRefs.
I'd suggest adding debug representation in a follow-up change, but also happy to do it now if you feel that's necessary.

That would need updates to all hover tests and that's a big enough to go into a separate change, from my perspective.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58547