dgoldman marked an inline comment as done.
dgoldman added inline comments.
================
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();
+ }
----------------
sammccall wrote:
> 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.
Thanks, that makes sense - I agree that B should be the actual canonical
declaration for our case (even though Clang picks A) and C is the definition. I
think that part was confusing me, since Clang seems to use the forward
declaration as the canonical one when it exists, but it seems like that's
intentional/OK and shouldn't change (e.g. for `ReferenceFinder` in the same
file).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83501/new/
https://reviews.llvm.org/D83501
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
