[PATCH] D41102: Setup clang-doc frontend framework

[Jake Ehrlich via Phabricator via cfe-commits]

jakehehrlich added a comment.

After the comments I just made are resolved, I'm fine with the low level details of the parts of this change that are here



================
Comment at: clang-doc/ClangDoc.cpp:47
+
+comments::FullComment *ClangDocCallback::getComment(const NamedDecl *D) {
+  RawComment *Comment = Context->getRawCommentForDeclNoCache(D);
----------------
I don't see any reason this can't be a const method. If I recall a previous version you said that it can be it can't be const because it modifies the Comment but that shouldn't violate the this being a const method. No modifications are being made to any members of this object and no non-const references/pointers to any of the members are accessed or needed.


================
Comment at: clang-doc/ClangDocMapper.cpp:91
+  ClangDocCommentVisitor Visitor(CI);
+  return Visitor.parseComment(C);
+}
----------------
If/When you make CurrentCI a reference you should return CI here instead.


================
Comment at: clang-doc/ClangDocMapper.cpp:181
+  for (comments::Comment *Child : make_range(C->child_begin(), C->child_end())) {
+    CommentInfo ChildCI;
+    ClangDocCommentVisitor Visitor(ChildCI);
----------------
Assuming you make the reference changes above, this should be rewritten to something like the following:

CurrentCI.Children.emplace_back();
ClangDocCommentVisitor Visitor(CurrentCI.Children.back());
Visitor.parseComment(Child);


================
Comment at: clang-doc/ClangDocMapper.cpp:266
+bool ClangDocCommentVisitor::isWhitespaceOnly(StringRef S) const {
+  return S.find_first_not_of(" \t\n\v\f\r") == std::string::npos;
+}
----------------
Can you use isspace here instead of keeping a list of characters that are considered spaces?


================
Comment at: clang-doc/ClangDocMapper.h:112
+  
+  CommentInfo parseComment(const comments::Comment *C);
+
----------------
This shouldn't return CommentInfo if you make CurrentCI a reference


================
Comment at: clang-doc/ClangDocMapper.h:129
+  
+  CommentInfo CurrentCI;
+};
----------------
This should be a CommentInfo& to avoid copy constructing. It also then lets you view ClangDocCommentVisitor as a kind of CommentInfo builder


https://reviews.llvm.org/D41102