[PATCH] D63857: [clang-doc] Structured HTML generator

Thu Jun 27 14:31:55 PDT 2019

jakehehrlich added a comment.

Overall looks better. One of my comments causes a sweeping change to occur so I'll respond back after thats done.

================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:91
+struct HTMLFile {
+  llvm::SmallString<16> DoctypeDecl{"<!DOCTYPE html>"};
+  std::vector<std::unique_ptr<HTMLNode>> Children; // List of child nodes
----------------
Does this ever change? If not this should be a global constexpr constant char* inside of an anon-namespace.

e.g.

namespace {
  constexpr const char* kDoctypeDecl = "...";
}

================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:170
+    OS << " " << A.getKey() << "=\"" << A.getValue() << "\"";
+  OS << (SelfClosing ? "/>" : ">");
+  if (!InlineChildren)
----------------
You can exit here if its self closing right?

================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:187

-std::string genTag(const Twine &Text, const Twine &Tag) {
-  return "<" + Tag.str() + ">" + Text.str() + "</" + Tag.str() + ">";
+static void genHTML(const EnumInfo &I, TagNode *N);
+static void genHTML(const FunctionInfo &I, TagNode *N);
----------------
You should return TagNodes, not assign to them by ref. Preferably via retruning a unique pointer. It doesn't make sense to have the consumer do the constructor I would imagine. For instance what tag is the right tag type? The consumer shouldn't decide, the producer should. 

================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:190
+
+static void genEnumsBlock(const std::vector<EnumInfo> &Enums, TagNode *N) {
+  if (Enums.empty())
----------------
Same comment here these function should look like (at a very high an imprecise level, obviously some functions will be more complicated.)

```
std::unique_ptr<TagNode> genFoo(info) {
  auto out = llvm::make_unique<...>(...);
  for (const auto &B : info.bars)
    out->Children.emplace_back(genBar(B));
  return out;
}
```

This goes for all of these functions

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

https://reviews.llvm.org/D63857