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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
