sammccall added inline comments.
================
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:
> 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)
>
>
> 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?
Oh right... yes, this seems fine to me (for the ObjCContainerDecl subclasses
only, and with a comment)
> 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)
I'm not sure what "this" in "this is actually a DeclRelation::Underlying"
refers to. Do you have a failing testcase?
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
