[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

Additional note: This diff is a diff from your last commit not the full diff relative to origin/master which is what should be up here.



================
Comment at: tools/clang-doc/ClangDoc.cpp:34
 
-bool ClangDocVisitor::VisitNamespaceDecl(const NamespaceDecl *D) {
+template <>
+void ClangDocCallback::processMatchedDecl(
----------------
I can't think of a good way to dedup these two methods at the moment. Can you put a TODO here to deduplicate these two specializations?


================
Comment at: tools/clang-doc/ClangDoc.h:69
 
-private:
-  ClangDocVisitor Visitor;
-  ClangDocReporter &Reporter;
-};
-
-class ClangDocAction : public clang::ASTFrontendAction {
-public:
-  ClangDocAction(ClangDocReporter &Reporter) : Reporter(Reporter) {}
-
-  virtual std::unique_ptr<clang::ASTConsumer>
-  CreateASTConsumer(clang::CompilerInstance &C, llvm::StringRef InFile);
+  virtual void HandleTranslationUnit(clang::ASTContext &Context) {
+    Finder->matchAST(Context);
----------------
This should be moved to the .cpp file. Because there is no key function (https://itanium-cxx-abi.github.io/cxx-abi/abi.html#vague-vtable) this method will be redefined in every translation unit that includes this header.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:422
+void ClangDocReporter::serializeLLVM(StringRef RootDir) {
+  // TODO: Implement.
+}
----------------
jakehehrlich wrote:
> Can you report an error to the user that says something along the lines of "not implemented yet" (leave the TODO as well)
I think it would be better if instead of returning a string, you just fail and print a message to the user (well, first print the message and then fail).


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:41
+  Docs.Files[Filename] = llvm::make_unique<File>();
+  Docs.Files[Filename]->Filename = Filename;
 }
----------------
nit: Can this just do one lookup?

```
auto F = llvm::make_unique<File>();
F->Filename = Filename;
Docs.Files[Filename] = std::move(Filename);
```


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:136
+  if (NS == Docs.Namespaces.end()) {
+    Docs.Namespaces[Namespace] = llvm::make_unique<NamespaceInfo>();
+    Docs.Namespaces[Namespace]->FullyQualifiedName = Namespace;
----------------
nit: could you rewrite with a single lookup.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:190
     return;
-  CommentInfo CI;
-  parseFullComment(C, CI);
-  I.Descriptions.push_back(CI);
+  I.Descriptions.push_back(std::move(parseFullComment(C)));
 }
----------------
If you use emplace_back here you don't need the explicit std::move


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:281-288
+  sys::path::native(RootDir, FilePath);
+  sys::path::append(FilePath, "files.yaml");
+  raw_fd_ostream FileOS(FilePath, OutErrorInfo, sys::fs::F_None);
+  if (OutErrorInfo != OK) {
+    errs() << OutErrorInfo.message();
+    errs() << " Error opening documentation file.\n";
+    return;
----------------
You use the same basic code 3 times for different file names. Can you factor that out into a function? Also in this block you output the OutErrorInfo message but in blocks below you don't. You should always output that message.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:335
+  RootCI->Kind = C->getCommentKindName();
+  CurrentCI = RootCI.get();
+  ConstCommentVisitor<ClangDocCommentVisitor>::visit(C);
----------------
Instead of assigning a CI like this, could you construct a new ClangDocCommentVisitor on the stack? The idea would be that you could would still have a "CI" member variable that would be set in the ClangDocCommentVisitor's constructor. That way it never has to change and each visitor is just responsible for constructing one CommentInfo




================
Comment at: tools/clang-doc/ClangDocReporter.h:168
 private:
+  template <typename T, class C>
+  void createInfo(llvm::StringMap<std::unique_ptr<T>> &InfoMap, const C *D,
----------------
If you add a public "using DeclType = FooDecl;" to each "FooInfo" you can eliminate the second template argument and make the intent of this code more clear. This also formalizes the connection these types have to each other.


================
Comment at: tools/clang-doc/tool/ClangDocMain.cpp:76
+  if (DirectoryStatus != OK) {
+    errs() << "Unable to create documentation directories.\n";
+    return 1;
----------------
Can you convert this error_code to a message and display that to the user?


https://reviews.llvm.org/D41102