[PATCH] D41102: Setup clang-doc frontend framework

[Jonas Toth via Phabricator via cfe-commits]

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