dgoldman marked 7 inline comments as done. dgoldman added a comment. 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. ================ 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: > > > > > > > > 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? ================ Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:371 + // TU, because in practice they are definitions. + if (shouldRecordDeclaration(*ND)) { + if (isPreferredDeclaration(*OriginalDecl, Roles)) ---------------- sammccall wrote: > Hmm, not sure about this. What if the @implementation is in a different file > we're not indexing? Then we'll end up with no declaration. > > Probably instead we should treat this as if it were a non-preferred > non-canonical declaration whose canonical declaration is the @interface. > i.e. around where FOK is handled: > ``` > if (auto *OCID = llvm::dyn_cast<ObjCImplDecl>(D)) > D = OCID->getClassInterface(); // hmm, but maybe not if it's an implicit > @interface > ``` Ok, I've done something like this but it gets odd since we need to keep track that it is canonical. LMK if I should swap this to be later to replace OriginalDecl or isPreferredDeclaration. 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
