[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

If its possible to split VisitEnumDecl, and VisitRecordDecl into separate methods and the same is possible for VisitFunctionDecl and VisitCXXMethodDecl then I think all of your methods will look like the following VisitNamespaceDecl. That being the case you might want to factor this out somehow (which I think also would resolve my comment about isUnparsed being used the same way a lot).

for instance you might have a function like

  template <class T>
  bool ClangDocVisitor::visitDecl(const T *D) {
    if (!isUnparsed(D->getLocation()))
      return true;
    Reporter.createInfo(D, getComment(D), getLine(D), getFile(D)); // Use explicit template specialization to make "createInfo" uniform
    return true;
  }

and then define a macro like the following

  #define DEFINE_VISIT_METHOD(TYPE) \
  bool ClangDocVisitor::Visit##TYPE(const TYPE *D) { return visitDecl(D); }



================
Comment at: tools/clang-doc/ClangDoc.cpp:22
+
+bool ClangDocVisitor::VisitTagDecl(const TagDecl *D) {
+  if (!isUnparsed(D->getLocation()))
----------------
Is it possible to use VisitEnumDecl and VisitRecordDecl separately here?


================
Comment at: tools/clang-doc/ClangDoc.cpp:35
+
+  // Error?
+  return true;
----------------
I think you should use llvm_unrechable here


================
Comment at: tools/clang-doc/ClangDoc.cpp:40
+bool ClangDocVisitor::VisitNamespaceDecl(const NamespaceDecl *D) {
+  if (!isUnparsed(D->getLocation()))
+    return true;
----------------
It looks like you're using this pattern a lot. It might be worth factoring this out somehow.


================
Comment at: tools/clang-doc/ClangDoc.cpp:46
+
+bool ClangDocVisitor::VisitFunctionDecl(const FunctionDecl *D) {
+  if (!isUnparsed(D->getLocation()))
----------------
Can you separate this into VisitFunctionDecl and VisitCXXMethodDecl?


================
Comment at: tools/clang-doc/ClangDoc.cpp:60
+
+comments::FullComment *ClangDocVisitor::getComment(const Decl *D) {
+  RawComment *Comment = Context->getRawCommentForDeclNoCache(D);
----------------
Can this be a const method?


================
Comment at: tools/clang-doc/ClangDoc.cpp:76
+
+std::string ClangDocVisitor::getFile(const Decl *D) const {
+  PresumedLoc PLoc = Manager.getPresumedLoc(D->getLocStart());
----------------
I think this method should return a StringRef instead of an std::string because the const char* returned by getFilename should live at least as long as the source manager.


================
Comment at: tools/clang-doc/ClangDoc.cpp:85
+      continue;
+    std::shared_ptr<CommentInfo> CI = std::make_shared<CommentInfo>();
+    Reporter.parseFullComment(Comment->parse(*Context, nullptr, nullptr), CI);
----------------
So haven't looked enough at the reporter code yet but it seems to me this should a unique pointer. You seem to already be aware of that based on a TODO I saw in the reporter code though. Is it possible that "parseFullComent" should just take a plain old pointer instead of a unique_ptr or shared_ptr? 


================
Comment at: tools/clang-doc/ClangDoc.cpp:92
+
+bool ClangDocVisitor::isUnparsed(SourceLocation Loc) const {
+  if (!Loc.isValid())
----------------
Can you add a comment documenting what this function does?


================
Comment at: tools/clang-doc/ClangDoc.cpp:108
+  if (const auto *Ctor = dyn_cast<CXXConstructorDecl>(D))
+    MC->mangleCXXCtor(Ctor, CXXCtorType::Ctor_Complete, MangledName);
+  else if (const auto *Dtor = dyn_cast<CXXDestructorDecl>(D))
----------------
I think it's kind of annoying that this can't be a const method because of these mangle calls. I don't really understand why MangleContext works the way that it does but it could be that this is a situation where the "mutable" keyword should be used on MC to allow what should be a const method to actully be const. That might be something to look into.


================
Comment at: tools/clang-doc/ClangDoc.cpp:113
+    MC->mangleName(D, MangledName);
+  return MangledName.str();
+}
----------------
I think you want to return S here so that the move constructor is used instead. str() returns a reference to S which will cause the copy constructor to be called. I *think* most std::string implementations have a copy on write optimization but it's strictly more ideal to use the move constructor.


================
Comment at: tools/clang-doc/ClangDoc.cpp:123
+ClangDocAction::CreateASTConsumer(CompilerInstance &C, StringRef InFile) {
+  return make_unique<ClangDocConsumer>(&C.getASTContext(), Reporter);
+}
----------------
Pro Tip: Always explicitly refer to this as "llvm::make_unique" because you'll have to revert this change if you don't.

Some of the build bots have C++14 headers instead of C++11 headers. This means that llvm::make_unique and std::make_unique will both be defined. This means that using "make_unique" will cause an error even though only llvm::make_unique can be referred to unqualified. So even if you're inside of the llvm namespace you should explicitly refer to "llvm::make_unique" and never use "make_unique".


================
Comment at: tools/clang-doc/ClangDocReporter.h:190
+
+  std::shared_ptr<CommentInfo> CurrentCI;
+  Documentation Docs;
----------------
This seems like a code smell to me. I haven't read though the usage of CurrentCI well enough yet to properly conclude what to do about this but I have an idea. Do you think it's possible to have another class that includes the methods that use CurrentCI? The other alternative might be to pass this just to where it's needed as that's basically what you're doing.

This all said, I haven't read most of this code yet so feel free to disregard this for right now.


https://reviews.llvm.org/D41102