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@Foo"}]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME:
"relationships":[{"kind":"memberOf","source":"c:@S@Foo@FI@bar","target":"c:@S@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@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@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
