[PATCH] D83501: [clangd][ObjC] Improve xrefs for protocols and classes

[David Goldman via Phabricator via cfe-commits]

dgoldman updated this revision to Diff 277026.
dgoldman added a comment.

Fixes for getDefinition/getCanonicalDecl for ObjC


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83501

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


Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -688,7 +688,14 @@
 
       R"objc(//objc
         @class $decl[[Foo]];
-        @interface $def[[Foo]]
+        Fo^o * getFoo() {
+          return 0;
+        }
+      )objc",
+
+      R"objc(//objc
+        @class Foo;
+        @interface $decl[[Foo]]
         @end
         Fo^o * getFoo() {
           return 0;
@@ -696,8 +703,8 @@
       )objc",
 
       R"objc(//objc
-        @class $decl[[Foo]];
-        @interface Foo
+        @class Foo;
+        @interface $decl[[Foo]]
         @end
         @implementation $def[[Foo]]
         @end
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -80,16 +80,8 @@
     return FD->getDefinition();
   if (const auto *PD = dyn_cast<ObjCProtocolDecl>(D))
     return PD->getDefinition();
-  // Objective-C classes can have three types of declarations:
-  //
-  // - forward declaration: @class MyClass;
-  // - definition declaration: @interface MyClass ... @end
-  // - implementation: @implementation MyClass ... @end
-  if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D)) {
-    if (const auto *IMD = ID->getImplementation())
-      return IMD;
-    return ID->getDefinition();
-  }
+  if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D))
+    return ID->getImplementation();
   // Only a single declaration is allowed.
   if (isa<ValueDecl>(D) || isa<TemplateTypeParmDecl>(D) ||
       isa<TemplateTemplateParmDecl>(D)) // except cases above
@@ -235,6 +227,30 @@
   return llvm::None;
 }
 
+// A wrapper around `Decl::getCanonicalDecl` to support cases where Clang's
+// definition of a canonical declaration doesn't match up to what a programmer
+// would expect. For example, Objective-C classes can have three types of
+// declarations:
+//
+// - forward declaration(s): @class MyClass;
+// - definition declaration: @interface MyClass ... @end
+// - implementation: @implementation MyClass ... @end
+//
+// Clang will consider the forward declaration to be the canonical declaration
+// because it is first, but we actually want the class definition if it is
+// available since that is what a programmer would consider the real definition
+// to be.
+const NamedDecl *getNonStandardCanonicalDecl(const NamedDecl *D) {
+  D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
+
+  // Prefer Objective-C class definitions over the forward declaration.
+  if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D))
+    if (const auto *DefinitionID = ID->getDefinition())
+      return DefinitionID;
+
+  return D;
+}
+
 // Decls are more complicated.
 // The AST contains at least a declaration, maybe a definition.
 // These are up-to-date, and so generally preferred over index results.
@@ -250,7 +266,7 @@
   llvm::DenseMap<SymbolID, size_t> ResultIndex;
 
   auto AddResultDecl = [&](const NamedDecl *D) {
-    D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
+    D = getNonStandardCanonicalDecl(D);
     auto Loc =
         makeLocation(AST.getASTContext(), nameLocation(*D, SM), MainFilePath);
     if (!Loc)


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D83501.277026.patch
Type: text/x-patch
Size: 3338 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200710/ce05f1b1/attachment.bin>