[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added inline comments.


================
Comment at: tools/clang-doc/ClangDoc.h:33
+
+class ClangDocVisitor : public RecursiveASTVisitor<ClangDocVisitor> {
+public:
----------------
sammccall wrote:
> This API makes essentially everything public. Is that the intent?
> 
> It seems like `ClangDocVisitor` is a detail, and the operation you want to expose is "extract doc from this AST into this reporter" or maybe "create an AST consumer that feeds this reporter".
> 
> It would be useful to have an API to extract documentation from individual AST nodes (e.g. a Decl). But I'd be nervous about trying to use the classes exposed here to do that. If it's efficiently possible, it'd be nice to expose a function.
> (one use case for this is clangd)
Correct me if I'm wrong but I believe that everything needs to be public in this case because the base class needs to be able to call them. So the visit methods all need to be public.


================
Comment at: tools/clang-doc/ClangDoc.h:39
+
+  bool VisitTagDecl(const TagDecl *D);
+  bool VisitNamespaceDecl(const NamespaceDecl *D);
----------------
sammccall wrote:
> `override` where applicable
These methods are not virtual methods. It's technically legal to use the override keyword if a subclass shadows a non-virtual method but I don't think its what we want to do here.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:43
+  F->Filename = Filename;
+  Docs.Files.insert(std::make_pair(Filename, std::move(F)));
+}
----------------
instead of inserting a pair can we just use '[]' syntax?


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:114
+  if (Pair == Docs.Namespaces[Namespace]->Functions.end()) {
+    std::unique_ptr<FunctionInfo> I = make_unique<FunctionInfo>();
+    Docs.Namespaces[Namespace]->Functions[MangledName] = std::move(I);
----------------
no need for I, and use llvm::make_unique


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:127
+
+void ClangDocReporter::createMethodInfo(const CXXMethodDecl *D,
+                                        const FullComment *C, int LineNumber,
----------------
As I'm looking though these methods more I'm thinking you might want to break each of these createInfo methods up into smaller parts. For instance the addLocation/addComment part is the same in everyone of these, they all extract some name from a decl, they all use that string to get an iterator to the needed item. They all check to see if that iterator is the end and then add the item to the container etc... There's a lot more opportunity for deduplication if  break these things up some.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:142
+  if (Pair == Docs.Types[ParentName]->Functions.end()) {
+    std::unique_ptr<FunctionInfo> I = make_unique<FunctionInfo>();
+    Docs.Types[ParentName]->Functions[MangledName] = std::move(I);
----------------
ditto again


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:155
+
+void ClangDocReporter::parseFullComment(const FullComment *C,
+                                        std::shared_ptr<CommentInfo> &CI) {
----------------
Do we need this method?


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:178
+    NamedType T{Attr.Name, Attr.Value, clang::AccessSpecifier::AS_none};
+    CurrentCI->Attrs.push_back(T);
+  }
----------------
Can we use emplace_back here instead of copying a NamedType?


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:303
+  CI->Kind = C->getCommentKindName();
+  ConstCommentVisitor<ClangDocReporter>::visit(C);
+  for (comments::Comment *Child :
----------------
So it looks like the reason you need CurrentCI is because the visit methods need it and you need different CI's to be used at different visit calls but the visit methods can't take any more parameters. I think you should put the visit methods in another class that takes a pointer to a CommentInfo as an argument to the constructor. I *think* that should clean up this code smell and help mitigate the use of shared_ptr everywhere.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:355
+  yaml::Output YOut(OS);
+  for (auto &I : Map)
+    YOut << *(I.second);
----------------
can these be const auto&?


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:363
+  yaml::Output YOut(OS);
+  for (auto &I : Map) {
+    YOut << *(I.second);
----------------
ditto on these


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:372
+  if (RootDir.empty()) {
+    printMap<std::unique_ptr<File>>(outs(), Docs.Files);
+    printMapPlusFunctions<std::unique_ptr<NamespaceInfo>>(outs(),
----------------
Do we need to explicitly pass these types? I think template argument deduction should fill this in for us.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:383-390
+  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() << "Error opening documentation file.\n";
+    return;
+  }
----------------
Can you factor this out into a function and dedup the code below?


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:390
+  }
+  printMap<std::unique_ptr<File>>(FileOS, Docs.Files);
+
----------------
ditto on explicit tempalte argument.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:422
+void ClangDocReporter::serializeLLVM(StringRef RootDir) {
+  // TODO: Implement.
+}
----------------
Can you report an error to the user that says something along the lines of "not implemented yet" (leave the TODO as well)


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:425
+
+const char *ClangDocReporter::getCommandName(unsigned CommandID) const {
+  const CommandInfo *Info = CommandTraits::getBuiltinCommandInfo(CommandID);
----------------
I think my personal preference (not a universally followed preference) is to use a StringRef because at some point you might very well need a StringRef.


================
Comment at: tools/clang-doc/ClangDocReporter.cpp:434
+bool ClangDocReporter::isWhitespaceOnly(StringRef S) const {
+  return S.find_first_not_of(" \t\n\v\f\r") == std::string::npos || S.empty();
+}
----------------
Rather than rolling this on your own you should use S.find_if_not(std::isspace). Also if S is empty then find_* functions should always return npos right? If that's the case then you don't need S.empty(). 


================
Comment at: tools/clang-doc/ClangDocReporter.h:48
+/// A representation of a parsed comment.
+struct CommentInfo {
+  std::string Kind;
----------------
There are a lot of std::string members here, do we know for sure that we CommentInfo to own all of these? My general strategy is to avoid owning data (e.g. use StringRef and ArrayRef) unless there's a good reason to own the data. This is a general question I have about all FooInfo structs.


https://reviews.llvm.org/D41102