JonasToth added a comment.
Hi julie,
i like the new approach for doc generator. I added my thoughts on the code :)
================
Comment at: tools/clang-doc/ClangDoc.cpp:81
+ llvm::StringRef InFile) {
+ return std::unique_ptr<ASTConsumer>(
+ new ClangDocConsumer(&Compiler.getASTContext(), Reporter));
----------------
`llvm::make_unique` is cleaner here.
================
Comment at: tools/clang-doc/ClangDoc.h:30
+ // Which format to emit representation in.
+ std::string EmitFormat;
+};
----------------
Is this a string for a discrete set of configurations? If so maybe a `enum
class` would be a better fit.
================
Comment at: tools/clang-doc/ClangDocReporter.cpp:91
+ClangDocReporter::ClangDocReporter(std::vector<std::string> SourcePathList) {
+ for (std::string Path : SourcePathList)
+ AddFile(Path);
----------------
That loop will copy the string in every iteration. Is this intended?
Same with the function: It will copy the whole list. I think at least one of
both copies is not necessary.
================
Comment at: tools/clang-doc/ClangDocReporter.cpp:96
+void ClangDocReporter::AddComment(StringRef Filename, CommentInfo &CI) {
+ FileRecords[Filename].UnattachedComments.push_back(std::move(CI));
+}
----------------
That move will move from the reference. That has the effect, that the call site
can not use `CI` anymore because its moved from. (same in `AddDecl`). Is this
intended? Even if it is, that might be very subtle. Taking by value might be a
better option here.
================
Comment at: tools/clang-doc/ClangDocReporter.cpp:106
+ FI.Filename = Filename;
+ FileRecords.insert(std::pair<StringRef, FileRecord>(Filename,
std::move(FI)));
+}
----------------
`std::make_pair(FileName, std::move(FI))` would be shorter.
================
Comment at: tools/clang-doc/ClangDocReporter.cpp:205
+ ConstCommentVisitor<ClangDocReporter>::visit(C);
+ for (comments::Comment::child_iterator I = C->child_begin(),
+ E = C->child_end();
----------------
`comments::Comment` could get a `childs()` method returning a view to iterate
with nice range based loops.
================
Comment at: tools/clang-doc/ClangDocReporter.cpp:236
+
+bool ClangDocReporter::isWhitespaceOnly(std::string S) {
+ return S.find_first_not_of(" \t\n\v\f\r") == std::string::npos || S.empty();
----------------
will copy `S`. using a `const&` removes the potential allocation
================
Comment at: tools/clang-doc/tool/ClangDocMain.cpp:69
+ llvm::outs() << "Writing docs...\n";
+ Reporter.Serialize(EmitFormat, llvm::outs());
+}
----------------
final `return 0;` for main is missing.
https://reviews.llvm.org/D41102
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
