[PATCH] D63857: [clang-doc] Add a structured HTML generator

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

This looks good to me!



================
Comment at: clang-tools-extra/clang-doc/Generators.cpp:75-77
 extern volatile int YAMLGeneratorAnchorSource;
 extern volatile int MDGeneratorAnchorSource;
+extern volatile int HTMLGeneratorAnchorSource;
----------------
I don't fully understand these lines. Can you explain how they work and what they accomplish?



================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:75
+    Children.emplace_back(
+        llvm::make_unique<TextNode>(Text.str(), !InlineChildren));
+  }
----------------
I think you can just write `Children.emplace_back(Text.str(), !InlineChildren)`


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:310-312
+    std::unique_ptr<HTMLNode> Node = genHTML(Child);
+    if (Node)
+      CommentBlock->Children.emplace_back(std::move(Node));
----------------
```
for (...)  
  if (std::unique)ptr<...> Node = genHTML(Child))
    CommentBlock->...
```

would be a bitmore idiomatic LLVM code. I think you used this pattern above as well so you should fix it there as well.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:325
+
+  Out.emplace_back(
+      llvm::make_unique<TagNode>(HTMLTag::TAG_H3, EnumType + I.Name));
----------------
same thing here I think.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:358
+
+  Out.emplace_back(llvm::make_unique<TagNode>(
+      HTMLTag::TAG_P, Access + I.ReturnType.Type.Name + " " + I.Name + "(" +
----------------
ditto


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:422
+    if (Parents.empty())
+      Out.emplace_back(llvm::make_unique<TagNode>(HTMLTag::TAG_P,
+                                                  "Inherits from " + VParents));
----------------
ditto


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:425
+    else if (VParents.empty())
+      Out.emplace_back(llvm::make_unique<TagNode>(HTMLTag::TAG_P,
+                                                  "Inherits from " + Parents));
----------------
ditto


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:428
+    else
+      Out.emplace_back(llvm::make_unique<TagNode>(
+          HTMLTag::TAG_P, "Inherits from " + Parents + ", " + VParents));
----------------
ditto


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:432-434
+  std::vector<std::unique_ptr<TagNode>> Members =
+      genRecordMembersBlock(I.Members);
+  std::move(Members.begin(), Members.end(), std::back_inserter(Out));
----------------
You use this a lot here and once or twice above. You could write a template function that takes an `std::vector<B>&&` and moves its contents into some other `std::vector<A>&` where B : A

You can use SFINAE to ensure that B is subtype of A.




================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:472-475
+    std::vector<std::unique_ptr<TagNode>> Nodes =
+        genHTML(*static_cast<clang::doc::NamespaceInfo *>(I), InfoTitle);
+    std::move(Nodes.begin(), Nodes.end(),
+              std::back_inserter(MainContentNode->Children));
----------------
These are all cases of what I was talking about before.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:505
+
+  F.Children.emplace_back(
+      llvm::make_unique<TagNode>(HTMLTag::TAG_TITLE, InfoTitle));
----------------
ditto.


================
Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:507
+      llvm::make_unique<TagNode>(HTMLTag::TAG_TITLE, InfoTitle));
+  F.Children.push_back(std::move(MainContentNode));
+
----------------
Use `emplace_back` when moving to avoid extra constructor calls.


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

https://reviews.llvm.org/D63857