[PATCH] D146656: [clang][ExtractAPI] Refactor ExtractAPIVisitor to make it more extensible

[Zixu Wang via Phabricator via cfe-commits]

zixuw added a comment.

Good to see refactoring to make ExtractAPI easier to extend and use. Thanks Daniel!
Some comments on the ExtractAPIVisitor part. I'll leave the libclang part to the experts.



================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:32
+template <typename Derived>
+class ExtractAPIVisitorBase : public RecursiveASTVisitor<Derived> {
 public:
----------------
Would it be better to call this `ExtractAPIVisitorImpl` because it provides the visitor implementation, and the `ExtractAPIVisitor` is actually the base for the second level CRTP for actual visitors?


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:32
+template <typename Derived>
+class ExtractAPIVisitorBase : public RecursiveASTVisitor<Derived> {
 public:
----------------
zixuw wrote:
> Would it be better to call this `ExtractAPIVisitorImpl` because it provides the visitor implementation, and the `ExtractAPIVisitor` is actually the base for the second level CRTP for actual visitors?
Should this be `: public RecursiveASTVisitor<ExtractAPIVisitorBase<Derived>>` instead?


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:34
 public:
-  ExtractAPIVisitor(ASTContext &Context,
-                    llvm::unique_function<bool(SourceLocation)> LocationChecker,
-                    APISet &API)
-      : Context(Context), API(API),
-        LocationChecker(std::move(LocationChecker)) {}
+  ExtractAPIVisitorBase(ASTContext &Context, APISet &API)
+      : Context(Context), API(API) {}
----------------
Should the constructor be made `protected` for a CRTP base?


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:57
+
+  const RawComment *fetchRawCommentForDecl(const Decl *Decl) const;
+
----------------
Why does comment-fetching need to be dispatched?


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:102
+private:
+  Derived &getConcrete() { return *static_cast<Derived *>(this); }
+};
----------------
I see that the `RecursiveASTVisitor` already provides a `getDerived()` for the top-level CRTP.
My head is still bending trying to get a clear view of the multi-level CRTP here 🙃 , but I'm guessing that this is to avoid the conflict with that method.
In that case could we be more verbose to make clear the purpose of this one? I'm thinking something like `getDerivedExtractAPIVisitor()`, to communicate that this is for getting the derived/concrete ExtractAPIVisitor subclass.


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:170
   bool operator()(SourceLocation Loc) {
-    // If the loc refers to a macro expansion we need to first get the file
+    // If the loc refersSourceLocationxpansion we need to first get the file
     // location of the expansion.
----------------
bnbarham wrote:
> Accidental?
Missed change?


================
Comment at: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp:228-234
+    // Check that we have the definition for redeclarable types.
+    if (auto *TD = llvm::dyn_cast<TagDecl>(D))
+      ShouldBeIncluded = TD->isThisDeclarationADefinition();
+    else if (auto *Interface = llvm::dyn_cast<ObjCInterfaceDecl>(D))
+      ShouldBeIncluded = Interface->isThisDeclarationADefinition();
+    else if (auto *Protocol = llvm::dyn_cast<ObjCProtocolDecl>(D))
+      ShouldBeIncluded = Protocol->isThisDeclarationADefinition();
----------------
Couldn't find the original logic in this patch. Could you remind me what are these for? Also good to have more comments here to explain things.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146656/new/

https://reviews.llvm.org/D146656