[PATCH] D71248: [clangd] Introduce paragraph, the first part of new rendering structs

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Design question around newlines :-) Otherwise looks good.



================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:95
+// Concatanates whitespace blocks into a single ` `.
+std::string canonicalizeSpaces(std::string Input) {
+  // Goes over the string and preserves only a single ` ` for any whitespace
----------------
kadircet wrote:
> sammccall wrote:
> > is this equivalent to
> > ```
> > SmallVector<StringRef> Words;
> > llvm::SplitString(Input, Words);
> > return llvm::join(Input, " ");
> > ```
> > ?
> > 
> > (That would trim leading/trailing whitespace, but I suspect we actually want that)
> yes that's exactly what we want, thanks!
> not just joining them though, since we could still merge them in-place.
I'm really not sure saving the allocation is worth the code here, but up to you.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:118
 
-std::string FormattedString::renderAsMarkdown() const {
+enum RenderType {
+  PlainText,
----------------
(why not just pass in the fptr?)


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:122
+};
+std::string renderBlocks(llvm::ArrayRef<std::unique_ptr<Block>> Children,
+                         RenderType RT) {
----------------
I'm not sure this common codepath for plain-text and markdown will survive in the long run:
 - state around indentation and wrapping is likely to differ
 - separating blocks with one newline may not be the right thing in each case


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:130
+    OS << Sep;
+    Sep = "\n";
+    ((*C).*RenderFunc)(OS);
----------------
for markdown, this means consecutive paragraphs will be rendered as a single paragraph with line breaks in it. intended?


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:139
+public:
+  void asMarkdown(llvm::raw_ostream &OS) const override {}
+  void asPlainText(llvm::raw_ostream &OS) const override {}
----------------
this deserves a comment - the effect is produced by the surrounding newlines.
But if we change the blocks to include their whitespace it'll be more obvious.


================
Comment at: clang-tools-extra/clangd/FormattedString.h:29
 public:
+  virtual void asMarkdown(llvm::raw_ostream &OS) const = 0;
+  virtual void asPlainText(llvm::raw_ostream &OS) const = 0;
----------------
Do these include trailing newline(s), or not?

Based on offline discussion, they currently do not, but this causes some problems e.g. list followed by paragraph would get merged.

Option a) have Blocks include required trailing newlines (e.g. \n\n after lists in markdown, \n after paragraphs), and have Document trim trailing newlines.
Option b) have Document look at the type of the elements above/below and insert space appropriately.

Personally prefer option A because it encapsulates behavior better in the classes (less dyn_cast) and I think reflects the grammar better.


================
Comment at: clang-tools-extra/clangd/Hover.cpp:460
   if (!Definition.empty()) {
-    Output.appendCodeBlock(Definition);
+    Output.addParagraph().appendCode(Definition);
   } else {
----------------
This is a regression at the moment - complicated definitions are clang-formatted, and we'll canonicalize the whitespace.
(The behavior of the library is correct, we just need code block for this).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248