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

[Daniel Grumberg via Phabricator via cfe-commits]

dang added inline comments.


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:32
+template <typename Derived>
+class ExtractAPIVisitorBase : public RecursiveASTVisitor<Derived> {
 public:
----------------
zixuw wrote:
> 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?
I chose to name it "Base" as this seems to be the convention used for ADT, where they use the "Base" suffix for types clients shouldn't be using.

As with the base class, we need to provide `RecursiveASTVisitor` the most derived class so that when it does the `getDerived()` dance it can use the most derived methods.


================
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) {}
----------------
zixuw wrote:
> Should the constructor be made `protected` for a CRTP base?
agreed


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:57
+
+  const RawComment *fetchRawCommentForDecl(const Decl *Decl) const;
+
----------------
zixuw wrote:
> Why does comment-fetching need to be dispatched?
This is for the libclang use case. If you request symbol graph data for a symbol in an implementation block for example you should get back the doc comment from the header. The main reason for doing this refactoring is so that we can avoid dynamic dispatch when doing source location lookups and now fetching documentation comments.


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:102
+private:
+  Derived &getConcrete() { return *static_cast<Derived *>(this); }
+};
----------------
zixuw wrote:
> 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.
Not fully sure myself, but I think that using the one from `RecursiveASTVisitor` would work fine here. I will give it a go and change it if it works.


================
Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:628
+public:
+  ExtractAPIVisitor(ASTContext &Context, APISet &API) : Base(Context, API) {}
+
----------------
zixuw wrote:
> Same as above, should the constructor be `protected`? I guess it depends if we want people to just instantiate and use a `ExtractAPIVisitor`.
The aim was for clients to be able to instantiate this one, hence why we go through a fair amount of effort to provide base class the most derived type with the `std::conditional_t` usage above.


================
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.
----------------
zixuw wrote:
> bnbarham wrote:
> > Accidental?
> Missed change?
yup definitely accidental will fix.


================
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();
----------------
zixuw wrote:
> 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.
I moved this behavior out of the base type into here, we used to do these checks in the individual `VisitXXX` methods and bail early, but I needed this behavior to be configurable for the libclang work. I figured it semantically belonged here.


================
Comment at: clang/test/Index/extract-api-cursor.m:36
 
-// RUN: c-index-test -single-symbol-sgfs local %s | FileCheck %s
-
-// Checking for Foo
-// CHECK: "parentContexts":[]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[]
-// CHECK-SAME: "text":"Foo docs"
-// CHECK-SAME: "kind":{"displayName":"Structure","identifier":"objective-c.struct"}
-// CHECK-SAME: "title":"Foo"
-
-// Checking for bar
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.struct","name":"Foo","usr":"c:@S at Foo"}]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:@S at Foo@FI at bar","target":"c:@S at Foo"
-// CHECK-SAME: "text":"Bar docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
-// CHECK-SAME: "title":"bar"
-
-// Checking for Base
-// CHECK-NEXT: "parentContexts":[]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[]
-// CHECK-SAME: "text":"Base docs"
-// CHECK-SAME: "kind":{"displayName":"Class","identifier":"objective-c.class"}
-// CHECK-SAME: "title":"Base"
-
-// Checking for baseProperty
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.class","name":"Base","usr":"c:objc(cs)Base"}]
-// CHECK-SAME: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
-// CHECK-SAME: "isSystem":false
-// CHECK-SAME: "usr":"c:@S at Foo"}]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Base(py)baseProperty","target":"c:objc(cs)Base"
-// CHECK-SAME: "text":"Base property docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
-// CHECK-SAME: "title":"baseProperty"
-
-// Checking for baseMethodWithArg
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.class","name":"Base","usr":"c:objc(cs)Base"}]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Base(im)baseMethodWithArg:","target":"c:objc(cs)Base"
-// CHECK-SAME: "text":"Base method docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Method","identifier":"objective-c.method"}
-// CHECK-SAME: "title":"baseMethodWithArg:"
-
-// Checking for Protocol
-// CHECK-NEXT: "parentContexts":[]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[]
-// CHECK-SAME: "text":"Protocol docs"
-// CHECK-SAME: "kind":{"displayName":"Protocol","identifier":"objective-c.protocol"}
-// CHECK-SAME: "title":"Protocol"
-
-// Checking for protocolProperty
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.protocol","name":"Protocol","usr":"c:objc(pl)Protocol"}]
-// CHECK-SAME: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
-// CHECK-SAME: "isSystem":false
-// CHECK-SAME: "usr":"c:@S at Foo"}]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:objc(pl)Protocol(py)protocolProperty","target":"c:objc(pl)Protocol"
-// CHECK-SAME: "text":"Protocol property docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
-// CHECK-SAME: "title":"protocolProperty"
-
-// Checking for Derived
-// CHECK-NEXT: "parentContexts":[]
-// CHECK-SAME: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
-// CHECK-SAME: "isSystem":false
-// CHECK-SAME: "usr":"c:objc(cs)Base"}]
-// CHECK-SAME: "relationships":[{"kind":"inheritsFrom","source":"c:objc(cs)Derived","target":"c:objc(cs)Base"
-// CHECK-SAME: "text":"Derived docs"
-// CHECK-SAME: "kind":{"displayName":"Class","identifier":"objective-c.class"}
-// CHECK-SAME: "title":"Derived"
-
-// Checking for derivedMethodWithValue
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.class","name":"Derived","usr":"c:objc(cs)Derived"}]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Derived(im)derivedMethodWithValue:","target":"c:objc(cs)Derived"
-// CHECK-SAME: "text":"Derived method docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Method","identifier":"objective-c.method"}
-// CHECK-SAME: "title":"derivedMethodWithValue:"
-
-// CHECK-NOT: This won't show up in docs because we can't serialize it
-// CHECK-NOT: Derived method in category docs, won't show up either.
+// RUN: c-index-test -single-symbol-sgf-at=%s:4:9 local %s | FileCheck -check-prefix=CHECK-FOO %s
+// CHECK-FOO: "parentContexts":[]
----------------
bnbarham wrote:
> I personally like to put the run lines next to what's being checked and use [[@LINE+1]]. Up to you though
Oh I didn't know about this that sounds like it's definitely more readable, will make the swap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146656