[PATCH] D71063: [clangd] New rendering structs

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/FormattedString.h:25
 /// plaintext upon requrest.
-class FormattedString {
+class RenderableString {
 public:
----------------
Naming: not sure "string" is the right name - it doesn't represent the things here that are stringiest.

Block? (maybe in a `markup` namespace?)


================
Comment at: clang-tools-extra/clangd/FormattedString.h:34
+
+/// Leaf type of the struct representation.
+class Paragraph : public RenderableString {
----------------
Comments throughout should probably refer to the semantics (what part of markup is this) rather than its role in the tree.


================
Comment at: clang-tools-extra/clangd/FormattedString.h:67
 
+/// Container for a set of documents. Each document is prepended with a "- " and
+/// separated by newlines.
----------------
"List" and "Container" commonly mean other things in C++, so I got confused. `BulletList`?

The comment "Container for a list of documents" describes its structure in the tree rather than its semantics, which most callers (markup composers) will care about. Maybe "A bulleted list. Each item is a child Document"?

I think the comment "each document is prepended..." belongs on renderAsPlainText() if at all


================
Comment at: clang-tools-extra/clangd/FormattedString.h:70
+class List : public RenderableString {
+public:
+  std::string renderAsMarkdown() const override;
----------------
obvious attribute here is bullet style (bullets vs numbers) - but you probably only need one for now


================
Comment at: clang-tools-extra/clangd/FormattedString.h:82
+/// Top-level container for structured strings.
+class Document : public RenderableString {
+public:
----------------
Document doesn't need to inherit from RenderableString AFAICS, it doesn't need to be embedded in Documents itself, just Lists.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71063