[PATCH] D114058: [clang] Add ObjC decls to Decl::isFunctionOrFunctionTemplate

[Sheldon Neuberger via Phabricator via cfe-commits]

sheldonneuberger-sc added a comment.

Thanks for the detailed comments. I moved the patch into the clangd callsite. I 
also added a couple ObjC tests for CallHierarchy (I basically parameterized two 
of the existing tests).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114058

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

[PATCH] D114058: [clang] Add ObjC decls to Decl::isFunctionOrFunctionTemplate

[Sheldon Neuberger via Phabricator via cfe-commits]

sheldonneuberger-sc updated this revision to Diff 388101.
sheldonneuberger-sc added a comment.

- fix test name


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114058

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp

Index: clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -51,6 +51,13 @@
 using ::testing::Matcher;
 using ::testing::UnorderedElementsAre;
 
+void verifyIncomingMultiFile(std::string SourceExt, std::string HeaderExt,
+ Annotations , Annotations ,
+ Annotations , Annotations ,
+ Annotations , Annotations ,
+ Annotations );
+void verifyIncomingOneFile(Annotations , std::string Filename);
+
 // Helpers for matching call hierarchy data structures.
 MATCHER_P(WithName, N, "") { return arg.name == N; }
 MATCHER_P(WithSelectionRange, R, "") { return arg.selectionRange == R; }
@@ -65,25 +72,11 @@
UnorderedElementsAre(M...));
 }
 
-TEST(CallHierarchy, IncomingOneFile) {
-  Annotations Source(R"cpp(
-void call^ee(int);
-void caller1() {
-  $Callee[[callee]](42);
-}
-void caller2() {
-  $Caller1A[[caller1]]();
-  $Caller1B[[caller1]]();
-}
-void caller3() {
-  $Caller1C[[caller1]]();
-  $Caller2[[caller2]]();
-}
-  )cpp");
+void verifyIncomingOneFile(Annotations , std::string Filename) {
   TestTU TU = TestTU::withCode(Source.code());
+  TU.Filename = Filename;
   auto AST = TU.build();
   auto Index = TU.index();
-
   std::vector Items =
   prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
   ASSERT_THAT(Items, ElementsAre(WithName("callee")));
@@ -91,7 +84,6 @@
   ASSERT_THAT(IncomingLevel1,
   ElementsAre(AllOf(From(WithName("caller1")),
 FromRanges(Source.range("Callee");
-
   auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get());
   ASSERT_THAT(IncomingLevel2,
   ElementsAre(AllOf(From(WithName("caller2")),
@@ -109,6 +101,45 @@
   EXPECT_THAT(IncomingLevel4, IsEmpty());
 }
 
+TEST(CallHierarchy, IncomingOneFileCpp) {
+  Annotations Source(R"cpp(
+void call^ee(int);
+void caller1() {
+  $Callee[[callee]](42);
+}
+void caller2() {
+  $Caller1A[[caller1]]();
+  $Caller1B[[caller1]]();
+}
+void caller3() {
+  $Caller1C[[caller1]]();
+  $Caller2[[caller2]]();
+}
+  )cpp");
+  verifyIncomingOneFile(Source, "TestTU.cpp");
+}
+
+TEST(CallHierarchy, IncomingOneFileObjC) {
+  Annotations Source(R"objc(
+@implementation MyClass {}
+  +(void)call^ee {
+}
++(void) caller1 {
+  [MyClass $Callee[[callee]]];
+}
++(void) caller2 {
+  [MyClass $Caller1A[[caller1]]];
+  [MyClass $Caller1B[[caller1]]];
+}
++(void) caller3 {
+  [MyClass $Caller1C[[caller1]]];
+  [MyClass $Caller2[[caller2]]];
+}
+@end
+  )objc");
+  verifyIncomingOneFile(Source, "TestTU.m");
+}
+
 TEST(CallHierarchy, MainFileOnlyRef) {
   // In addition to testing that we store refs to main-file only symbols,
   // this tests that anonymous namespaces do not interfere with the
@@ -172,7 +203,7 @@
 FromRanges(Source.range("Caller2");
 }
 
-TEST(CallHierarchy, IncomingMultiFile) {
+TEST(CallHierarchy, IncomingMultiFileCpp) {
   // The test uses a .hh suffix for header files to get clang
   // to parse them in C++ mode. .h files are parsed in C mode
   // by default, which causes problems because e.g. symbol
@@ -215,15 +246,84 @@
 }
   )cpp");
 
-  TestWorkspace Workspace;
-  Workspace.addSource("callee.hh", CalleeH.code());
-  Workspace.addSource("caller1.hh", Caller1H.code());
-  Workspace.addSource("caller2.hh", Caller2H.code());
-  Workspace.addMainFile("callee.cc", CalleeC.code());
-  Workspace.addMainFile("caller1.cc", Caller1C.code());
-  Workspace.addMainFile("caller2.cc", Caller2C.code());
-  Workspace.addMainFile("caller3.cc", Caller3C.code());
+  verifyIncomingMultiFile(".cc", ".hh", CalleeH, Caller1H, Caller2H, CalleeC,
+  Caller1C, Caller2C, Caller3C);
+}
+
+TEST(CallHierarchy, IncomingMultiFileObjC) {
+  // The test uses a .mi suffix for header files to get clang
+  // to parse them in ObjC mode. .h files are parsed in C mode
+  // by default, which causes problems because e.g. symbol
+  // USRs are different in C mode (do not include function signatures).
+
+  Annotations CalleeH(R"objc(
+@interface CalleeClass
+  +(void)call^ee;
+@end
+  )objc");
+  Annotations CalleeC(R"objc(
+#import "callee.mi"
+@implementation CalleeClass {}
+  

[PATCH] D114058: [clang] Add ObjC decls to Decl::isFunctionOrFunctionTemplate

[Sheldon Neuberger via Phabricator via cfe-commits]

sheldonneuberger-sc updated this revision to Diff 388100.
sheldonneuberger-sc added a comment.

- clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114058

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp

Index: clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -51,6 +51,13 @@
 using ::testing::Matcher;
 using ::testing::UnorderedElementsAre;
 
+void verifyIncomingMultiFile(std::string SourceExt, std::string HeaderExt,
+ Annotations , Annotations ,
+ Annotations , Annotations ,
+ Annotations , Annotations ,
+ Annotations );
+void verifyIncomingOneFile(Annotations , std::string Filename);
+
 // Helpers for matching call hierarchy data structures.
 MATCHER_P(WithName, N, "") { return arg.name == N; }
 MATCHER_P(WithSelectionRange, R, "") { return arg.selectionRange == R; }
@@ -65,25 +72,11 @@
UnorderedElementsAre(M...));
 }
 
-TEST(CallHierarchy, IncomingOneFile) {
-  Annotations Source(R"cpp(
-void call^ee(int);
-void caller1() {
-  $Callee[[callee]](42);
-}
-void caller2() {
-  $Caller1A[[caller1]]();
-  $Caller1B[[caller1]]();
-}
-void caller3() {
-  $Caller1C[[caller1]]();
-  $Caller2[[caller2]]();
-}
-  )cpp");
+void verifyIncomingOneFile(Annotations , std::string Filename) {
   TestTU TU = TestTU::withCode(Source.code());
+  TU.Filename = Filename;
   auto AST = TU.build();
   auto Index = TU.index();
-
   std::vector Items =
   prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
   ASSERT_THAT(Items, ElementsAre(WithName("callee")));
@@ -91,7 +84,6 @@
   ASSERT_THAT(IncomingLevel1,
   ElementsAre(AllOf(From(WithName("caller1")),
 FromRanges(Source.range("Callee");
-
   auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get());
   ASSERT_THAT(IncomingLevel2,
   ElementsAre(AllOf(From(WithName("caller2")),
@@ -109,6 +101,45 @@
   EXPECT_THAT(IncomingLevel4, IsEmpty());
 }
 
+TEST(CallHierarchy, IncomingOneFile) {
+  Annotations Source(R"cpp(
+void call^ee(int);
+void caller1() {
+  $Callee[[callee]](42);
+}
+void caller2() {
+  $Caller1A[[caller1]]();
+  $Caller1B[[caller1]]();
+}
+void caller3() {
+  $Caller1C[[caller1]]();
+  $Caller2[[caller2]]();
+}
+  )cpp");
+  verifyIncomingOneFile(Source, "TestTU.cpp");
+}
+
+TEST(CallHierarchy, IncomingOneFileObjC) {
+  Annotations Source(R"objc(
+@implementation MyClass {}
+  +(void)call^ee {
+}
++(void) caller1 {
+  [MyClass $Callee[[callee]]];
+}
++(void) caller2 {
+  [MyClass $Caller1A[[caller1]]];
+  [MyClass $Caller1B[[caller1]]];
+}
++(void) caller3 {
+  [MyClass $Caller1C[[caller1]]];
+  [MyClass $Caller2[[caller2]]];
+}
+@end
+  )objc");
+  verifyIncomingOneFile(Source, "TestTU.m");
+}
+
 TEST(CallHierarchy, MainFileOnlyRef) {
   // In addition to testing that we store refs to main-file only symbols,
   // this tests that anonymous namespaces do not interfere with the
@@ -172,7 +203,7 @@
 FromRanges(Source.range("Caller2");
 }
 
-TEST(CallHierarchy, IncomingMultiFile) {
+TEST(CallHierarchy, IncomingMultiFileCpp) {
   // The test uses a .hh suffix for header files to get clang
   // to parse them in C++ mode. .h files are parsed in C mode
   // by default, which causes problems because e.g. symbol
@@ -215,15 +246,84 @@
 }
   )cpp");
 
-  TestWorkspace Workspace;
-  Workspace.addSource("callee.hh", CalleeH.code());
-  Workspace.addSource("caller1.hh", Caller1H.code());
-  Workspace.addSource("caller2.hh", Caller2H.code());
-  Workspace.addMainFile("callee.cc", CalleeC.code());
-  Workspace.addMainFile("caller1.cc", Caller1C.code());
-  Workspace.addMainFile("caller2.cc", Caller2C.code());
-  Workspace.addMainFile("caller3.cc", Caller3C.code());
+  verifyIncomingMultiFile(".cc", ".hh", CalleeH, Caller1H, Caller2H, CalleeC,
+  Caller1C, Caller2C, Caller3C);
+}
 
+TEST(CallHierarchy, IncomingMultiFileObjC) {
+  // The test uses a .mi suffix for header files to get clang
+  // to parse them in ObjC mode. .h files are parsed in C mode
+  // by default, which causes problems because e.g. symbol
+  // USRs are different in C mode (do not include function signatures).
+
+  Annotations CalleeH(R"objc(
+@interface CalleeClass
+  +(void)call^ee;
+@end
+  )objc");
+  Annotations CalleeC(R"objc(
+#import "callee.mi"
+@implementation CalleeClass {}
+  

[PATCH] D114058: [clang] Add ObjC decls to Decl::isFunctionOrFunctionTemplate

[Sheldon Neuberger via Phabricator via cfe-commits]

sheldonneuberger-sc updated this revision to Diff 388099.
sheldonneuberger-sc added a comment.
Herald added a subscriber: arphaman.
Herald added a project: clang-tools-extra.

- move change to clangd callsite
- add tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114058

Files:
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp

Index: clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/CallHierarchyTests.cpp
@@ -51,6 +51,9 @@
 using ::testing::Matcher;
 using ::testing::UnorderedElementsAre;
 
+void verifyIncomingMultiFile(std::string SourceExt, std::string HeaderExt, Annotations , Annotations , Annotations , Annotations , Annotations , Annotations , Annotations );
+void verifyIncomingOneFile(Annotations , std::string Filename);
+
 // Helpers for matching call hierarchy data structures.
 MATCHER_P(WithName, N, "") { return arg.name == N; }
 MATCHER_P(WithSelectionRange, R, "") { return arg.selectionRange == R; }
@@ -65,25 +68,11 @@
UnorderedElementsAre(M...));
 }
 
-TEST(CallHierarchy, IncomingOneFile) {
-  Annotations Source(R"cpp(
-void call^ee(int);
-void caller1() {
-  $Callee[[callee]](42);
-}
-void caller2() {
-  $Caller1A[[caller1]]();
-  $Caller1B[[caller1]]();
-}
-void caller3() {
-  $Caller1C[[caller1]]();
-  $Caller2[[caller2]]();
-}
-  )cpp");
+void verifyIncomingOneFile(Annotations , std::string Filename) {
   TestTU TU = TestTU::withCode(Source.code());
+  TU.Filename = Filename;
   auto AST = TU.build();
   auto Index = TU.index();
-
   std::vector Items =
   prepareCallHierarchy(AST, Source.point(), testPath(TU.Filename));
   ASSERT_THAT(Items, ElementsAre(WithName("callee")));
@@ -91,7 +80,6 @@
   ASSERT_THAT(IncomingLevel1,
   ElementsAre(AllOf(From(WithName("caller1")),
 FromRanges(Source.range("Callee");
-
   auto IncomingLevel2 = incomingCalls(IncomingLevel1[0].from, Index.get());
   ASSERT_THAT(IncomingLevel2,
   ElementsAre(AllOf(From(WithName("caller2")),
@@ -109,6 +97,45 @@
   EXPECT_THAT(IncomingLevel4, IsEmpty());
 }
 
+TEST(CallHierarchy, IncomingOneFile) {
+  Annotations Source(R"cpp(
+void call^ee(int);
+void caller1() {
+  $Callee[[callee]](42);
+}
+void caller2() {
+  $Caller1A[[caller1]]();
+  $Caller1B[[caller1]]();
+}
+void caller3() {
+  $Caller1C[[caller1]]();
+  $Caller2[[caller2]]();
+}
+  )cpp");
+  verifyIncomingOneFile(Source, "TestTU.cpp");
+}
+
+TEST(CallHierarchy, IncomingOneFileObjC) {
+  Annotations Source(R"objc(
+@implementation MyClass {}
+  +(void)call^ee {
+}
++(void) caller1 {
+  [MyClass $Callee[[callee]]];
+}
++(void) caller2 {
+  [MyClass $Caller1A[[caller1]]];
+  [MyClass $Caller1B[[caller1]]];
+}
++(void) caller3 {
+  [MyClass $Caller1C[[caller1]]];
+  [MyClass $Caller2[[caller2]]];
+}
+@end
+  )objc");
+  verifyIncomingOneFile(Source, "TestTU.m");
+}
+
 TEST(CallHierarchy, MainFileOnlyRef) {
   // In addition to testing that we store refs to main-file only symbols,
   // this tests that anonymous namespaces do not interfere with the
@@ -172,7 +199,7 @@
 FromRanges(Source.range("Caller2");
 }
 
-TEST(CallHierarchy, IncomingMultiFile) {
+TEST(CallHierarchy, IncomingMultiFileCpp) {
   // The test uses a .hh suffix for header files to get clang
   // to parse them in C++ mode. .h files are parsed in C mode
   // by default, which causes problems because e.g. symbol
@@ -215,15 +242,78 @@
 }
   )cpp");
 
-  TestWorkspace Workspace;
-  Workspace.addSource("callee.hh", CalleeH.code());
-  Workspace.addSource("caller1.hh", Caller1H.code());
-  Workspace.addSource("caller2.hh", Caller2H.code());
-  Workspace.addMainFile("callee.cc", CalleeC.code());
-  Workspace.addMainFile("caller1.cc", Caller1C.code());
-  Workspace.addMainFile("caller2.cc", Caller2C.code());
-  Workspace.addMainFile("caller3.cc", Caller3C.code());
+  verifyIncomingMultiFile(".cc", ".hh", CalleeH, Caller1H, Caller2H, CalleeC, Caller1C, Caller2C, Caller3C);
+}
 
+TEST(CallHierarchy, IncomingMultiFileObjC) {
+  // The test uses a .mi suffix for header files to get clang
+  // to parse them in ObjC mode. .h files are parsed in C mode
+  // by default, which causes problems because e.g. symbol
+  // USRs are different in C mode (do not include function signatures).
+
+  Annotations CalleeH(R"objc(
+@interface CalleeClass
+  +(void)call^ee;
+@end
+  )objc");
+  Annotations CalleeC(R"objc(
+#import "callee.mi"
+@implementation CalleeClass {}
+  +(void)call^ee {}
+@end
+  )objc");
+  

[PATCH] D114058: [clang] Add ObjC decls to Decl::isFunctionOrFunctionTemplate

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I agree with Nathan on this one. It's unclear why there are two functions that 
does the same thing in different ways and some usages in sema looks suspicious 
enough to require caution (there are subtle checks for shadowing and whatnot 
and I don't know how these rules come into play in objc contexts).

So I would suggest updating clangd side to look like:

  (isa(Decl) && cast(Decl)->isFunctionOrMethod()) || 
Decl->getlKind() == FunctionTemplate


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114058

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

[PATCH] D114058: [clang] Add ObjC decls to Decl::isFunctionOrFunctionTemplate

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the patch.

Could you add a test exercising call hierarchy for obj-c methods to 
CallHierarchyTests.cpp 
 please?

In terms of the actual change, this function has some other callers 
 in 
Parser and Sema code, and I don't know how this change will affect them. It may 
be better to limit the change to the clangd call site 

 for now (that is, change that call site to use a local helper function, which 
checks `isFunctionOrFunctionTemplate() || `), and defer changing 
`Decl::isFunctionOrFunctionTemplate()` itself to a subsequent refactor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114058

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

[PATCH] D114058: [clang] Add ObjC decls to Decl::isFunctionOrFunctionTemplate

[Sheldon Neuberger via Phabricator via cfe-commits]

sheldonneuberger-sc created this revision.
sheldonneuberger-sc added a reviewer: nridge.
Herald added subscribers: usaxena95, kadircet.
sheldonneuberger-sc added a reviewer: erichkeane.
sheldonneuberger-sc edited the summary of this revision.
sheldonneuberger-sc published this revision for review.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.

This fixes "textDocument/prepareCallHierarchy" in clangd for ObjC methods. 
Details at https://github.com/clangd/vscode-clangd/issues/247.

clangd uses Decl::isFunctionOrFunctionTemplate to check if the decl given in a 
prepareCallHierarchy request is eligible for prepareCallHierarchy, so we want 
it to return true for ObjC methods too. I added Block, Captured, and ObjCMethod 
because that's what was also done in DeclContext::isFunctionOrMethod.

Need guidance on if the function should be renamed to 
isFunctionOrMethodOrFunctionTemplate.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D114058

Files:
  clang/include/clang/AST/DeclBase.h


Index: clang/include/clang/AST/DeclBase.h
===
--- clang/include/clang/AST/DeclBase.h
+++ clang/include/clang/AST/DeclBase.h
@@ -1040,9 +1040,15 @@
 
   /// Whether this declaration is a function or function template.
   bool isFunctionOrFunctionTemplate() const {
-return (DeclKind >= Decl::firstFunction &&
-DeclKind <= Decl::lastFunction) ||
-   DeclKind == FunctionTemplate;
+switch (DeclKind) {
+case Decl::Block:
+case Decl::Captured:
+case Decl::ObjCMethod:
+case Decl::FunctionTemplate:
+  return true;
+default:
+  return DeclKind >= Decl::firstFunction && DeclKind <= Decl::lastFunction;
+}
   }
 
   /// If this is a declaration that describes some template, this


Index: clang/include/clang/AST/DeclBase.h
===
--- clang/include/clang/AST/DeclBase.h
+++ clang/include/clang/AST/DeclBase.h
@@ -1040,9 +1040,15 @@
 
   /// Whether this declaration is a function or function template.
   bool isFunctionOrFunctionTemplate() const {
-return (DeclKind >= Decl::firstFunction &&
-DeclKind <= Decl::lastFunction) ||
-   DeclKind == FunctionTemplate;
+switch (DeclKind) {
+case Decl::Block:
+case Decl::Captured:
+case Decl::ObjCMethod:
+case Decl::FunctionTemplate:
+  return true;
+default:
+  return DeclKind >= Decl::firstFunction && DeclKind <= Decl::lastFunction;
+}
   }
 
   /// If this is a declaration that describes some template, this
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits