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