sammccall added a comment. In D83501#2154300 <https://reviews.llvm.org/D83501#2154300>, @dgoldman wrote:
> In D83501#2153605 <https://reviews.llvm.org/D83501#2153605>, @sammccall wrote: > > > In D83501#2148671 <https://reviews.llvm.org/D83501#2148671>, @dgoldman > > wrote: > > > > > I implemented goto-definition from Foo() --> Foo impl in Xrefs, on the > > > symbolcollector side I don't think there's anything to do? > > > > > > This can't be done purely in xrefs as the AST may not be available. > > > > A.m: `@class Foo; ^Foo x;` <-- go-to-definition at ^ > > B.m: `@interface Foo...@end @implementation Foo...@end` > > > > The index needs to know that for the USR associated with the @class (found > > by targetDecl), the canonical decl is the @interface in B and the > > definition is the @implementation in B. > > So SymbolCollector needs to see it as a definition. **The tests seem to > > show it does already**, but it's not obvious why from the code. Do you > > know? Maybe it's the fact that they share a USR and thus a symbol ID. This > > is worth making explicit somewhere. > > > Think we're talking about different things. See `ObjCClassExtensions` in > SymbolCollectorTests which I added, the idea is that the `Cat ()` can link to > the implementation like I added to XRefs, but I'm not sure how this should be > represented. As for @class -> @interface/@implementation those should have > the same USR, yes. Oh, sorry I indeed missed the parens. Yes, this is a go-to-def special case, SymbolCollector shouldn't need anything. (I'm assuming that a reference to the class is reported already - could add a test that the ref exists if you like). ================ 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: > > > > > 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? > > > > 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. > > > > > > Not besides what was mentioned above, so I think it's OK at the moment. > > > > > > > I'm not sure what "this" in "this is actually a > > > > DeclRelation::Underlying" refers to. Do you have a failing testcase? > > > > > > I meant that for ObjC, I used `DeclRelation::Underlying` so I have to > > > include it to get the `ObjCCategoryImplDecl`. I guess I can later filter > > > out the `DeclRelation::Underlying` unless it's for the ObjC case. > > > > > > Failing test, I think only returning callback is intentional? > > > > > > ``` > > > LocateSymbol.TemplateTypedefs > > > > > > Value of: locateSymbolAt(AST, T.point()) > > > Expected: has 1 element that sym "callback" > > > Actual: { function: 1:30-1:38@file:///clangd-test/TestTU.cpp > > > def=1:30-1:38@file:///clangd-test/TestTU.cpp, callback: > > > 2:29-2:37@file:///clangd-test/TestTU.cpp }, which has 2 elements > > > > > > Code: > > > template <class T> struct function {}; > > > template <class T> using callback = function<T()>; > > > > > > c^allback<int> foo; > > > ``` > > > > > > > > > > > > > > > I meant that for ObjC, I used DeclRelation::Underlying > > > > `Underlying` isn't appropriate here - it's used for situations like > > typedefs where there's clearly two symbols (two names) and an asymmetrical > > synonym relation (e.g. int32 is a synonym for int, but not the other way > > around). > > Here we have a declaration/definition pair of a (what we're treating as) a > > single decl. > > > > Can you just stop setting `Underlying` in FindTargets? > If I do that it'll be filtered out since it's not a > DeclRelation::TemplatePattern | DeclRelation::Alias. Should I add a new > DeclRelation for this? I think one of us is confused about what these bitmasks do :-) - bits set on a particular decl *restrict* when that decl will be considered a target - bits passed to targetDecl() *allow* decls with those bits to be returned Not all targets found have a relation set, e.g. a DeclRefExpr to a variable doesn't have any of these bits. This means it's universal - it'll always be considered the target. Bits are basically set when we have to make a choice of whether to return some decl (if we're looking at the std::string type, then the std::string typedef is a target if we want aliases). 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
