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