[PATCH] D152720: [clangd][ObjC] Support ObjC class rename from implementation decls
David Goldman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 20 07:13:55 PDT 2023
dgoldman added inline comments.
================
Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:131
return HighlightingKind::Interface;
- if (isa<ObjCCategoryDecl>(D))
+ if (isa<ObjCCategoryDecl, ObjCCategoryImplDecl>(D))
return HighlightingKind::Namespace;
----------------
kadircet wrote:
> let's do this in a separate change, with some tests
Given the other changes, this is needed otherwise the semantic highlighting test fails.
================
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();
+ }
----------------
kadircet wrote:
> 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.
Replied to you in chat, LMK what you think.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152720/new/
https://reviews.llvm.org/D152720
More information about the cfe-commits
mailing list