[PATCH] D83501: [clangd][ObjC] Improve xrefs for protocols and classes

[David Goldman via Phabricator via cfe-commits]

dgoldman marked 2 inline comments as done.
dgoldman added inline comments.


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:83
+    return PD->getDefinition();
+  // Objective-C classes can have three types of declarations:
+  //
----------------
sammccall wrote:
> sammccall wrote:
> > This is a really useful comment, thanks!
> ... and it's gone.
> I think some comment is useful here, as this line is doing something subtly different than all the other lines - returning a decl that isn't equivalent to its input.
I had moved it to `getPreferredDecl`, will add it back here as well with more info (need to special case one more special category: a class extension)


================
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)) {
----------------
sammccall wrote:
> dgoldman wrote:
> > sammccall wrote:
> > > 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)
> > Whoops, meant to comment here but it was lost. I'm not sure what you meant here. Should this be done here inside the for loop or in getDeclAtPosition?
> > 
> > Are you saying that given:
> > 
> > ```
> > @interface Foo // A
> > @end
> > @implementation Foo // B
> > @end
> > ```
> > B --> A here
> > 
> > and similarly
> > 
> > ```
> > @interface Foo // A
> > @end
> > @interface Foo (Ext) // B
> > @end
> > @implementation Foo (Ext) // C
> > @end
> > ```
> > B --> A
> > C --> B (and A? it's unclear how this should map over, e.g. maybe Foo loc --> A, Ext --> B)
> In the first example, selecting either A or B should yield one LocatedSymbol with {Decl=A, Def=B}. This shouldn't require any special-casing.
> 
> The second example is very similar to template specialization, with exceptions:
>  - there's always a Decl/Def pair you may want to navigate between, whereas in templates there rarely is, so we have ambiguity
>  - there's no AST like there is for template names and args, just a bag of tokens
> 
> I'd suggest, given `@interface Foo (Ext)`:
>  - we produce a LocatedSymbol with {Decl=@interface Foo(Ext), Def=@implementation  Foo(Ext)} - this is the default behavior
>  - if the cursor is exactly on the token `Foo`, we also produce a LocatedSymbol with {Decl=@interface Foo, Def=@implementation Foo} - this is similar to the template special case
>  - if the cursor is exactly on the token Ext... are categories explicitly/separately declared anywhere? I guess not. If they are, we could special case this too.
> And `@implementation Foo(Ext)` should behave in exactly the same way.
Trying this out now, two problems:

- getDeclAtPosition will call findTarget. Due to the changes above we map `ObjCCategoryImplDecl` to its interface. This is OK but then when we check for the loc it's obviously != to the impl's loc. Should I modify this to check the contents of the loc for equality?

- We call `getDeclAtPosition` only looking for DeclRelation::TemplatePattern | DeclRelation::Alias, but this is actually a DeclRelation::Underlying. If I don't add that we filter out the ObjCCategoryImplDecl. If I add it we get a failure in LocateSymbol.TemplateTypedef (we now find another symbol, not sure what is intended here)




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83501