[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/FindTarget.cpp:715
+
+    void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) {
+      if (const auto *CI = OIMD->getClassInterface())
----------------
can you also add tests for these into `FindTargetTests`?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:716
+    void VisitObjCImplementationDecl(const ObjCImplementationDecl *OIMD) {
+      if (const auto *CI = OIMD->getClassInterface())
+        Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
----------------
we don't have the null check in other places, what's the significance here?


================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:131
     return HighlightingKind::Interface;
-  if (isa<ObjCCategoryDecl>(D))
+  if (isa<ObjCCategoryDecl, ObjCCategoryImplDecl>(D))
     return HighlightingKind::Namespace;
----------------
let's do this in a separate change, with some tests


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:171-177
+    if (const auto *C = dyn_cast<ObjCCategoryDecl>(D)) {
+      if (C->getLocation() == TokenStartLoc)
+        D = C->getClassInterface();
+      else if (const auto *I = C->getImplementation())
+        if (I->getLocation() == TokenStartLoc)
+          D = C->getClassInterface();
+    }
----------------
sorry i don't follow what's the logic doing here and we're likely doing these in the wrong layer.

we should either:
- Fix selection tree to pick the correct ASTNode, if it's picking the wrong one due to not having special cases for these locations here
- Fix the `targetDecl` logic to make sure it emits all the declarations that might be referenced by this ast node, if it's missing ClassInterface.
- Fix the canonicalRenameDecl, if we should always prefer `ClassInterface` in these cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152720