[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:164
                   AST.getHeuristicResolver())) {
     Result.insert(canonicalRenameDecl(D));
   }
----------------
before calling `canonicalRenameDecl` here we can do a `D = pickInterestingTarget(D);` and map it to interface decl once (we'll probably need something similar for propertydecls?), rather than canonicalizing all ObjcCategoryDecls. that way we'll still get to rename proper references that target ObjcInterfaceDecls.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:288
+            // the interface(s) which they reference.
+            if (isa<ObjCCategoryDecl, ObjCCategoryImplDecl>(Target))
+              continue;
----------------
i am afraid we're doing this at the wrong layer. the discrepancy is actually arising from the fact that `findExplicitReferences` will report name locations for an entity whose primary location contains a different name (e.g. `@interface ^Foo (^Bar)` saying that there's a reference to objccategorydecl at `Bar`, but canonicalizing it to `Foo`).

so i think what we really want is not to canonicalize objccategory(impl)decls to objcinterfacedecl, but rather pick rename target as objcinterfacedecl when the user is trying to rename a categorydecl inside `locatelDeclAt` (put more comments near that part of the code).

sorry for the extra round trip here :/


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:787
+  // names like class and protocol names.
+  if (const auto *CD = dyn_cast<ObjCContainerDecl>(&RenameDecl))
+    if (CD->getName() != IdentifierToken->text(SM))
----------------
this special casing should no longer be needed if we just map CategoryDecls to InterfaceDecls in `locateDeclAt` rather than at canonicalization time


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:1990
+
+  std::pair</*Code*/ llvm::StringRef, /*References*/ llvm::StringRef>
+      AllRefsCases[] = {// Simple expressions.
----------------
can you move this into a separate test case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152720