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