dgoldman marked 7 inline comments as done. dgoldman added a comment. In D83501#2157957 <https://reviews.llvm.org/D83501#2157957>, @sammccall wrote:
> 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). if I understand this correctly I think `MultipleDeclsWithSameDefinition` already tests that, although it relies upon `locateASTReferent` 's `TouchedIdentifier` being the interface. Should it always be added instead? ================ 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: > > > > 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). Haha yeah, should have just tried it. Didn't realize the masking style in `targetDecl` which filters from `allTargetDecls` always includes 0 Relations sets (since it's a not a normal filter, it's inverting). It works =) 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
