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

[Daniel Grumberg via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG21750a1ae8c8: [clang][ExtractAPI] Refactor ExtractAPIVisitor 
to make it more extensible (authored by dang).

Changed prior to commit:
  https://reviews.llvm.org/D146656?vs=507544=508696#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146656

Files:
  clang/test/Index/extract-api-cursor.m
  clang/tools/c-index-test/c-index-test.c

Index: clang/tools/c-index-test/c-index-test.c
===
--- clang/tools/c-index-test/c-index-test.c
+++ clang/tools/c-index-test/c-index-test.c
@@ -3,6 +3,7 @@
 #include "clang-c/BuildSystem.h"
 #include "clang-c/CXCompilationDatabase.h"
 #include "clang-c/CXErrorCode.h"
+#include "clang-c/CXSourceLocation.h"
 #include "clang-c/CXString.h"
 #include "clang-c/Documentation.h"
 #include "clang-c/Index.h"
@@ -4881,6 +4882,21 @@
   return result;
 }
 
+static void inspect_single_symbol_sgf_cursor(CXCursor Cursor) {
+  CXSourceLocation CursorLoc;
+  CXString SGFData;
+  const char *SGF;
+  unsigned line, column;
+  CursorLoc = clang_getCursorLocation(Cursor);
+  clang_getSpellingLocation(CursorLoc, 0, , , 0);
+  printf("%d:%d: ", line, column);
+
+  SGFData = clang_getSymbolGraphForCursor(Cursor);
+  SGF = clang_getCString(SGFData);
+  if (SGF)
+printf("%s\n", SGF);
+}
+
 /**/
 /* Command line processing.   */
 /**/
@@ -4940,6 +4956,7 @@
 "   c-index-test -print-usr-file \n");
   fprintf(stderr,
   "   c-index-test -single-symbol-sgfs  {*}\n"
+  "   c-index-test -single-symbol-sgf-at= {*}\n"
   "   c-index-test -single-symbol-sgf-for= {}*\n");
   fprintf(stderr,
 "   c-index-test -write-pch  \n"
@@ -5076,6 +5093,9 @@
   else if (argc > 3 && strcmp(argv[1], "-single-symbol-sgfs") == 0)
 return perform_test_load_source(argc - 3, argv + 3, argv[2],
 PrintSingleSymbolSGFs, NULL);
+  else if (argc > 2 && strstr(argv[1], "-single-symbol-sgf-at=") == argv[1])
+return inspect_cursor_at(
+argc, argv, "-single-symbol-sgf-at=", inspect_single_symbol_sgf_cursor);
   else if (argc > 2 && strstr(argv[1], "-single-symbol-sgf-for=") == argv[1])
 return perform_test_single_symbol_sgf(argv[1], argc - 2, argv + 2);
 
Index: clang/test/Index/extract-api-cursor.m
===
--- clang/test/Index/extract-api-cursor.m
+++ clang/test/Index/extract-api-cursor.m
@@ -1,3 +1,5 @@
+// Test is line- and column-sensitive. Run lines are below
+
 /// Foo docs
 struct Foo {
 /// Bar docs
@@ -25,91 +27,94 @@
 - (void)derivedMethodWithValue:(id)value;
 @end
 
-/// This won't show up in docs because we can't serialize it
-@interface Derived ()
-/// Derived method in category docs, won't show up either.
-- (void)derivedMethodInCategory;
+@implementation Derived
+- (void)derivedMethodWithValue:(id)value {
+int a = 5;
+}
 @end
 
-// 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
-// 

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

[Ben Barham via Phabricator via cfe-commits]

bnbarham added a comment.

> I have decided against doing that, because we can't specify @LINE in the 
> c-index-test command line.

FWIW you can do:

  // RUN:  -pos=%(line+1):7
  let bar = Bar()
  // CHECK ... [[@LINE-1]]:7

But I don't think this is particularly common, it's just how I write tests. 
What you have is perfectly reasonable 


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

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

[Daniel Grumberg via Phabricator via cfe-commits]

dang added a comment.

In D146656#4220022 , @zixuw wrote:

> LGTM for the `ExtractAPIVisitor` part.
> Remaining:
>
> - update test with `@LINE`
> - the libclang side

I have decided against doing that, because we can't specify `@LINE` in the 
`c-index-test` command line.
@bnbarham are you happy with the libclang bits?




Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:102
+private:
+  Derived () { return *static_cast(this); }
+};

dang wrote:
> 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.
turns out it is needed but not sure why, I will rename to make things clearer.



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: 

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

[Zixu Wang via Phabricator via cfe-commits]

zixuw added a comment.

LGTM for the `ExtractAPIVisitor` part.
Remaining:

- update test with `@LINE`
- the libclang side


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

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

[Daniel Grumberg via Phabricator via cfe-commits]

dang updated this revision to Diff 507544.
dang added a comment.

Adding back missing diffs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146656

Files:
  clang/include/clang/ExtractAPI/ExtractAPIVisitor.h
  clang/include/clang/ExtractAPI/TypedefUnderlyingTypeResolver.h
  clang/lib/ExtractAPI/CMakeLists.txt
  clang/lib/ExtractAPI/DeclarationFragments.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/ExtractAPI/ExtractAPIVisitor.cpp
  clang/lib/ExtractAPI/TypedefUnderlyingTypeResolver.cpp
  clang/lib/ExtractAPI/TypedefUnderlyingTypeResolver.h
  clang/test/Index/extract-api-cursor.m
  clang/tools/c-index-test/c-index-test.c
  clang/tools/libclang/CXExtractAPI.cpp

Index: clang/tools/libclang/CXExtractAPI.cpp
===
--- clang/tools/libclang/CXExtractAPI.cpp
+++ clang/tools/libclang/CXExtractAPI.cpp
@@ -18,6 +18,7 @@
 #include "clang-c/Index.h"
 #include "clang-c/Platform.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclObjC.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/ExtractAPI/API.h"
 #include "clang/ExtractAPI/ExtractAPIVisitor.h"
@@ -34,13 +35,73 @@
 using namespace clang;
 using namespace clang::extractapi;
 
+namespace {
+struct LibClangExtractAPIVisitor
+: ExtractAPIVisitor {
+  using Base = ExtractAPIVisitor;
+
+  LibClangExtractAPIVisitor(ASTContext , APISet )
+  : ExtractAPIVisitor(Context, API) {}
+
+  const RawComment *fetchRawCommentForDecl(const Decl *D) const {
+return Context.getRawCommentForAnyRedecl(D);
+  }
+
+  // We need to visit implementations as well to ensure that when a user clicks
+  // on a method defined only within the implementation that we can still
+  // provide a symbol graph for it.
+  bool VisitObjCImplementationDecl(const ObjCImplementationDecl *Decl) {
+if (!shouldDeclBeIncluded(Decl))
+  return true;
+
+const ObjCInterfaceDecl *Interface = Decl->getClassInterface();
+StringRef Name = Interface->getName();
+StringRef USR = API.recordUSR(Decl);
+PresumedLoc Loc =
+Context.getSourceManager().getPresumedLoc(Decl->getLocation());
+LinkageInfo Linkage = Decl->getLinkageAndVisibility();
+DocComment Comment;
+if (auto *RawComment = fetchRawCommentForDecl(Interface))
+  Comment = RawComment->getFormattedLines(Context.getSourceManager(),
+  Context.getDiagnostics());
+
+// Build declaration fragments and sub-heading by generating them for the
+// interface.
+DeclarationFragments Declaration =
+DeclarationFragmentsBuilder::getFragmentsForObjCInterface(Interface);
+DeclarationFragments SubHeading =
+DeclarationFragmentsBuilder::getSubHeading(Decl);
+
+// Collect super class information.
+SymbolReference SuperClass;
+if (const auto *SuperClassDecl = Decl->getSuperClass()) {
+  SuperClass.Name = SuperClassDecl->getObjCRuntimeNameAsString();
+  SuperClass.USR = API.recordUSR(SuperClassDecl);
+}
+
+ObjCInterfaceRecord *ObjCInterfaceRecord = API.addObjCInterface(
+Name, USR, Loc, AvailabilitySet(Decl), Linkage, Comment, Declaration,
+SubHeading, SuperClass, isInSystemHeader(Decl));
+
+// Record all methods (selectors). This doesn't include automatically
+// synthesized property methods.
+recordObjCMethods(ObjCInterfaceRecord, Decl->methods());
+recordObjCProperties(ObjCInterfaceRecord, Decl->properties());
+recordObjCInstanceVariables(ObjCInterfaceRecord, Decl->ivars());
+
+return true;
+  }
+};
+} // namespace
+
 DEFINE_SIMPLE_CONVERSION_FUNCTIONS(APISet, CXAPISet)
 
-static void WalkupFromMostDerivedType(ExtractAPIVisitor , Decl *D);
+static void WalkupFromMostDerivedType(LibClangExtractAPIVisitor ,
+  Decl *D);
 
 template 
 static bool WalkupParentContext(DeclContext *Parent,
-ExtractAPIVisitor ) {
+LibClangExtractAPIVisitor ) {
   if (auto *D = dyn_cast(Parent)) {
 WalkupFromMostDerivedType(Visitor, D);
 return true;
@@ -48,7 +109,8 @@
   return false;
 }
 
-static void WalkupFromMostDerivedType(ExtractAPIVisitor , Decl *D) {
+static void WalkupFromMostDerivedType(LibClangExtractAPIVisitor ,
+  Decl *D) {
   switch (D->getKind()) {
 #define ABSTRACT_DECL(DECL)
 #define DECL(CLASS, BASE)  \
@@ -84,8 +146,7 @@
   auto Lang = Unit->getInputKind().getLanguage();
   APISet *API = new APISet(Ctx.getTargetInfo().getTriple(), Lang,
Unit->getMainFileName().str());
-  ExtractAPIVisitor Visitor(
-  Ctx, [](SourceLocation Loc) { return true; }, *API);
+  LibClangExtractAPIVisitor Visitor(Ctx, *API);
 
   for (auto It = Unit->top_level_begin(); It != Unit->top_level_end(); ++It) {
 

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

[Daniel Grumberg via Phabricator via cfe-commits]

dang updated this revision to Diff 507543.
dang added a comment.

Addressing code review feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146656

Files:
  clang/include/clang/ExtractAPI/ExtractAPIVisitor.h
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp

Index: clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
===
--- clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
+++ clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
@@ -167,7 +167,7 @@
 
 struct LocationFileChecker {
   bool operator()(SourceLocation Loc) {
-// If the loc refersSourceLocationxpansion we need to first get the file
+// If the loc refers to a macro expansion we need to first get the file
 // location of the expansion.
 auto  = CI.getSourceManager();
 auto FileLoc = SM.getFileLoc(Loc);
Index: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h
===
--- clang/include/clang/ExtractAPI/ExtractAPIVisitor.h
+++ clang/include/clang/ExtractAPI/ExtractAPIVisitor.h
@@ -30,10 +30,11 @@
 
 template 
 class ExtractAPIVisitorBase : public RecursiveASTVisitor {
-public:
+protected:
   ExtractAPIVisitorBase(ASTContext , APISet )
   : Context(Context), API(API) {}
 
+public:
   const APISet () const { return API; }
 
   bool VisitVarDecl(const VarDecl *Decl);
@@ -99,7 +100,9 @@
   }
 
 private:
-  Derived () { return *static_cast(this); }
+  Derived () {
+return *static_cast(this);
+  }
 };
 
 template 
@@ -121,7 +124,7 @@
   Decl->getTemplateSpecializationKind() == TSK_Undeclared)
 return true;
 
-  if (!getConcrete().shouldDeclBeIncluded(Decl))
+  if (!getDerivedExtractAPIVisitor().shouldDeclBeIncluded(Decl))
 return true;
 
   // Collect symbol information.
@@ -131,7 +134,8 @@
   Context.getSourceManager().getPresumedLoc(Decl->getLocation());
   LinkageInfo Linkage = Decl->getLinkageAndVisibility();
   DocComment Comment;
-  if (auto *RawComment = getConcrete().fetchRawCommentForDecl(Decl))
+  if (auto *RawComment =
+  getDerivedExtractAPIVisitor().fetchRawCommentForDecl(Decl))
 Comment = RawComment->getFormattedLines(Context.getSourceManager(),
 Context.getDiagnostics());
 
@@ -183,7 +187,7 @@
 return true;
   }
 
-  if (!getConcrete().shouldDeclBeIncluded(Decl))
+  if (!getDerivedExtractAPIVisitor().shouldDeclBeIncluded(Decl))
 return true;
 
   // Collect symbol information.
@@ -193,7 +197,8 @@
   Context.getSourceManager().getPresumedLoc(Decl->getLocation());
   LinkageInfo Linkage = Decl->getLinkageAndVisibility();
   DocComment Comment;
-  if (auto *RawComment = getConcrete().fetchRawCommentForDecl(Decl))
+  if (auto *RawComment =
+  getDerivedExtractAPIVisitor().fetchRawCommentForDecl(Decl))
 Comment = RawComment->getFormattedLines(Context.getSourceManager(),
 Context.getDiagnostics());
 
@@ -214,7 +219,7 @@
 
 template 
 bool ExtractAPIVisitorBase::VisitEnumDecl(const EnumDecl *Decl) {
-  if (!getConcrete().shouldDeclBeIncluded(Decl))
+  if (!getDerivedExtractAPIVisitor().shouldDeclBeIncluded(Decl))
 return true;
 
   SmallString<128> QualifiedNameBuffer;
@@ -232,7 +237,8 @@
   PresumedLoc Loc =
   Context.getSourceManager().getPresumedLoc(Decl->getLocation());
   DocComment Comment;
-  if (auto *RawComment = getConcrete().fetchRawCommentForDecl(Decl))
+  if (auto *RawComment =
+  getDerivedExtractAPIVisitor().fetchRawCommentForDecl(Decl))
 Comment = RawComment->getFormattedLines(Context.getSourceManager(),
 Context.getDiagnostics());
 
@@ -247,7 +253,8 @@
   Comment, Declaration, SubHeading, isInSystemHeader(Decl));
 
   // Now collect information about the enumerators in this enum.
-  getConcrete().recordEnumConstants(EnumRecord, Decl->enumerators());
+  getDerivedExtractAPIVisitor().recordEnumConstants(EnumRecord,
+Decl->enumerators());
 
   return true;
 }
@@ -259,7 +266,7 @@
   if (isa(Decl))
 return true;
 
-  if (!getConcrete().shouldDeclBeIncluded(Decl))
+  if (!getDerivedExtractAPIVisitor().shouldDeclBeIncluded(Decl))
 return true;
 
   // Collect symbol information.
@@ -273,7 +280,8 @@
   PresumedLoc Loc =
   Context.getSourceManager().getPresumedLoc(Decl->getLocation());
   DocComment Comment;
-  if (auto *RawComment = getConcrete().fetchRawCommentForDecl(Decl))
+  if (auto *RawComment =
+  getDerivedExtractAPIVisitor().fetchRawCommentForDecl(Decl))
 Comment = RawComment->getFormattedLines(Context.getSourceManager(),
 Context.getDiagnostics());
 
@@ -288,7 +296,8 @@
 SubHeading, isInSystemHeader(Decl));
 
   // Now collect information about the fields in 

[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 
+class ExtractAPIVisitorBase : public RecursiveASTVisitor {
 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>` 
> 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 ,
-llvm::unique_function 
LocationChecker,
-APISet )
-  : Context(Context), API(API),
-LocationChecker(std::move(LocationChecker)) {}
+  ExtractAPIVisitorBase(ASTContext , APISet )
+  : 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 () { return *static_cast(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 , APISet ) : 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(D))
+  ShouldBeIncluded = TD->isThisDeclarationADefinition();
+else if (auto *Interface = llvm::dyn_cast(D))
+  ShouldBeIncluded = Interface->isThisDeclarationADefinition();
+else if (auto *Protocol = llvm::dyn_cast(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"}
-// 

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

[Zixu Wang via Phabricator via cfe-commits]

zixuw added inline comments.



Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:628
+public:
+  ExtractAPIVisitor(ASTContext , APISet ) : Base(Context, API) {}
+

Same as above, should the constructor be `protected`? I guess it depends if we 
want people to just instantiate and use a `ExtractAPIVisitor`.


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

[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 
+class ExtractAPIVisitorBase : public RecursiveASTVisitor {
 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 
+class ExtractAPIVisitorBase : public RecursiveASTVisitor {
 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>` 
instead?



Comment at: clang/include/clang/ExtractAPI/ExtractAPIVisitor.h:34
 public:
-  ExtractAPIVisitor(ASTContext ,
-llvm::unique_function 
LocationChecker,
-APISet )
-  : Context(Context), API(API),
-LocationChecker(std::move(LocationChecker)) {}
+  ExtractAPIVisitorBase(ASTContext , APISet )
+  : 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 () { return *static_cast(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(D))
+  ShouldBeIncluded = TD->isThisDeclarationADefinition();
+else if (auto *Interface = llvm::dyn_cast(D))
+  ShouldBeIncluded = Interface->isThisDeclarationADefinition();
+else if (auto *Protocol = llvm::dyn_cast(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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.



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.

Accidental?



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":[]

I personally like to put the run lines next to what's being checked and use 
[[@LINE+1]]. Up to you though


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  

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

[Daniel Grumberg via Phabricator via cfe-commits]

dang created this revision.
dang added reviewers: zixuw, ributzka, bnbarham.
Herald added subscribers: ChuanqiXu, arphaman.
Herald added a project: All.
dang requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Use CRTP to enable creating statically dispatched subclasses of
ExtractAPIVisitor.
This enables adding extension points and customising the behavior more
easily.

This is used in CXExtractAPI.cpp to create a specialized visitor for
Libclang as well as streamlining the batch implementation in 
ExtractAPIConsumer.cpp

[clang][ExtractAPI] Improve tests for clang_getSymbolGraphForCursor

Adds a new mode to c-index-test that can fetch a single symbol symbol
graph for a given source location. This way we can be more precise when
writing tests for clang_getSymbolGraphForCursor.
Additionaly this makes it easier to debug the function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D146656

Files:
  clang/include/clang/ExtractAPI/ExtractAPIVisitor.h
  clang/include/clang/ExtractAPI/TypedefUnderlyingTypeResolver.h
  clang/lib/ExtractAPI/CMakeLists.txt
  clang/lib/ExtractAPI/DeclarationFragments.cpp
  clang/lib/ExtractAPI/ExtractAPIConsumer.cpp
  clang/lib/ExtractAPI/ExtractAPIVisitor.cpp
  clang/lib/ExtractAPI/TypedefUnderlyingTypeResolver.cpp
  clang/lib/ExtractAPI/TypedefUnderlyingTypeResolver.h
  clang/test/Index/extract-api-cursor.m
  clang/tools/c-index-test/c-index-test.c
  clang/tools/libclang/CXExtractAPI.cpp

Index: clang/tools/libclang/CXExtractAPI.cpp
===
--- clang/tools/libclang/CXExtractAPI.cpp
+++ clang/tools/libclang/CXExtractAPI.cpp
@@ -18,6 +18,7 @@
 #include "clang-c/Index.h"
 #include "clang-c/Platform.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclObjC.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/ExtractAPI/API.h"
 #include "clang/ExtractAPI/ExtractAPIVisitor.h"
@@ -34,13 +35,73 @@
 using namespace clang;
 using namespace clang::extractapi;
 
+namespace {
+struct LibClangExtractAPIVisitor
+: ExtractAPIVisitor {
+  using Base = ExtractAPIVisitor;
+
+  LibClangExtractAPIVisitor(ASTContext , APISet )
+  : ExtractAPIVisitor(Context, API) {}
+
+  const RawComment *fetchRawCommentForDecl(const Decl *D) const {
+return Context.getRawCommentForAnyRedecl(D);
+  }
+
+  // We need to visit implementations as well to ensure that when a user clicks
+  // on a method defined only within the implementation that we can still
+  // provide a symbol graph for it.
+  bool VisitObjCImplementationDecl(const ObjCImplementationDecl *Decl) {
+if (!shouldDeclBeIncluded(Decl))
+  return true;
+
+const ObjCInterfaceDecl *Interface = Decl->getClassInterface();
+StringRef Name = Interface->getName();
+StringRef USR = API.recordUSR(Decl);
+PresumedLoc Loc =
+Context.getSourceManager().getPresumedLoc(Decl->getLocation());
+LinkageInfo Linkage = Decl->getLinkageAndVisibility();
+DocComment Comment;
+if (auto *RawComment = fetchRawCommentForDecl(Interface))
+  Comment = RawComment->getFormattedLines(Context.getSourceManager(),
+  Context.getDiagnostics());
+
+// Build declaration fragments and sub-heading by generating them for the
+// interface.
+DeclarationFragments Declaration =
+DeclarationFragmentsBuilder::getFragmentsForObjCInterface(Interface);
+DeclarationFragments SubHeading =
+DeclarationFragmentsBuilder::getSubHeading(Decl);
+
+// Collect super class information.
+SymbolReference SuperClass;
+if (const auto *SuperClassDecl = Decl->getSuperClass()) {
+  SuperClass.Name = SuperClassDecl->getObjCRuntimeNameAsString();
+  SuperClass.USR = API.recordUSR(SuperClassDecl);
+}
+
+ObjCInterfaceRecord *ObjCInterfaceRecord = API.addObjCInterface(
+Name, USR, Loc, AvailabilitySet(Decl), Linkage, Comment, Declaration,
+SubHeading, SuperClass, isInSystemHeader(Decl));
+
+// Record all methods (selectors). This doesn't include automatically
+// synthesized property methods.
+recordObjCMethods(ObjCInterfaceRecord, Decl->methods());
+recordObjCProperties(ObjCInterfaceRecord, Decl->properties());
+recordObjCInstanceVariables(ObjCInterfaceRecord, Decl->ivars());
+
+return true;
+  }
+};
+} // namespace
+
 DEFINE_SIMPLE_CONVERSION_FUNCTIONS(APISet, CXAPISet)
 
-static void WalkupFromMostDerivedType(ExtractAPIVisitor , Decl *D);
+static void WalkupFromMostDerivedType(LibClangExtractAPIVisitor ,
+  Decl *D);
 
 template 
 static bool WalkupParentContext(DeclContext *Parent,
-ExtractAPIVisitor ) {
+LibClangExtractAPIVisitor ) {
   if (auto *D = dyn_cast(Parent)) {
 WalkupFromMostDerivedType(Visitor, D);
 return true;
@@ -48,7 +109,8 @@
   return false;
 }
 
-static void