sammccall marked an inline comment as done.
sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:83
+ return PD->getDefinition();
+ // Objective-C classes can have three types of declarations:
+ //
----------------
This is a really useful comment, thanks!
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:88-92
+ if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D)) {
+ if (const auto *IMD = ID->getImplementation())
+ return IMD;
+ return ID->getDefinition();
+ }
----------------
dgoldman wrote:
> Let me know if there's a better way to handle this multi-"definition" support
I think there might be (what you've done seems reasonable in isolation but I'm
not sure it'll yield the best behavior).
Consider this code:
```
int foo(); // A
int foo(); // B
int foo(){} // C
```
We have 3 declarations. A will be chosen as the canonical declaration (because
it's first, though things get more complicated when the index is involved). C
is the definition. So go-to-definition will land on C, and then triggering it
again will take you to A, and then back to C. go-to-declaration is the
opposite. B is basically just a reference for our purposes, we won't navigate
you there (except by find-refs).
Now let's look at your example again:
```
@class MyClass; // A
@interface MyClass ... @end // B
@implementation MyClass ... @end // C
```
Thinking about where you might want to navigate to, A is certainly the least
important of these, right?
It seems clear we want B to be considered the canonical declaration and C the
definition.
So I think:
- we should only return the implementation here if it exists, and otherwise
nullptr rather than the inferface.
- getCanonicalDecl in AddResultDecl should special case ObjCInterfaceDecl to
get the definition (i.e. @interface) if possible
- we need to convince other parts of clangd that these are the same symbol:
- targetDecl should resolve any reference to an ObjCImplDecl to the
corresponding ObjCInterfaceDecl instead
- the indexer (`SymbolCollector`) also needs to handle this (the
ObjCImplDecl should end up being the stored Definition under the
ObjCInterfaceDecl's SymbolID)
Some code will only see the forward declaration and that's OK. The index's
merge rules are that a candidate "canonical declaration" which has an
associated definition is preferred over one that doesn't. Since the
implementation can always see the interface, the interface will end up being
the canonical declaration after merge.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:276
getDeclAtPosition(AST, CurLoc, Relations, NodeKind)) {
// Special case: void foo() ^override: jump to the overridden method.
if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) {
----------------
dgoldman wrote:
> Think it would make sense to special case ObjCInterfaceDecl here to get at
> both the interface definition + implementation if available?
Rather than returning both results, I think it's more consistent to return them
as a declaration/definition pair.
(This means special-casing ObjCImplDecl in namedDecl or at least
getDeclAsPosition, so you always end up with the ObjCInterfaceDecl instead)
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:723
+ std::string ObjcPrefix = "//objc";
+ if (strncmp(Test, ObjcPrefix.c_str(), ObjcPrefix.size()) == 0) {
+ TU.Filename = "TestTU.m";
----------------
Hm, I quite like this. You can probably also just do
TU.ExtraArgs.push_back("-xobjective-c++") though, it probably won't break
anything.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83501/new/
https://reviews.llvm.org/D83501
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
