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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked 12 inline comments as done.
kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:122
+};
+std::string renderBlocks(llvm::ArrayRef<std::unique_ptr<Block>> Children,
+                         RenderType RT) {
----------------
sammccall wrote:
> 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
I suppose this will survive in the long run now, as it only prints blocks, without separation, and trims in the end.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:130
+    OS << Sep;
+    Sep = "\n";
+    ((*C).*RenderFunc)(OS);
----------------
sammccall wrote:
> for markdown, this means consecutive paragraphs will be rendered as a single paragraph with line breaks in it. intended?
yes this was intended, but as each block takes care of its own indentation now, this is no longer an issue.


================
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;
----------------
sammccall wrote:
> 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.
taking option A


================
Comment at: clang-tools-extra/clangd/Hover.cpp:460
   if (!Definition.empty()) {
-    Output.appendCodeBlock(Definition);
+    Output.addParagraph().appendCode(Definition);
   } else {
----------------
sammccall wrote:
> 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).
not planning to land until we've got the complete implementation, including lists and codeblocks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71248