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