dgoldman marked 4 inline comments as done. dgoldman added a comment. In D83501#2146892 <https://reviews.llvm.org/D83501#2146892>, @sammccall wrote:
> In D83501#2144283 <https://reviews.llvm.org/D83501#2144283>, @dgoldman wrote: > > > Looking further into the indexing support, I've questions: > > > > - How `targetDecl` should behave for these objc container types > > > (Hmm, targetDecl doesn't seem to canonicalize decls anywhere, which might be > a bug, but it's unrelated to objc) > > The main thing is that AIUI @implementation is one thing, as far as clang's > concerned, while @class+@interface are another. > i.e. getCanonicalDecl() and USR generation both split them into equivalence > classes {@implementation} and {@class, @interface}. Is that right? I believe they share the same USR but besides that, yes that's correct. > In that case the thing targetDecl needs to do is "jump" from the > implementation to the interface, because we're pretending they're the same > decl. > It doesn't need to jump from the @class to the @interface, semantic > canonicalization doesn't belong there. (In fact it may need to do the > opposite, since targetDecl might be relying on implicitly canonicalizing with > clang's semantics). SGTM, done. >> - Similarly for SymbolCollector (guessing in >> `SymbolCollector::handleDeclOccurrence` ?) > > Right - this is where most of the real canonicalization happens. > You need to ensure that: > > - isPreferred is true for @interface, so that when we see both @class and > @interface we'll take the latter as our preferred declaration > - addDefinition gets called for @implementation, so that the definition > location is set correctly. Also the SymbolID used in this case must be based > on the USR of the interface, so we're considering them a single symbol. > (Either addDeclaration should not be called in this case, or it should be > called with the @implementation again somehow). I think that should be > enough... Gone ahead and implemented this. It seems OK. I think the only thing that might be missing here is class extensions: @interface Foo @end @interface Foo () - (void)bar; @end @implementation Foo - (void)bar {} @end I implemented goto-definition from Foo() --> Foo impl in Xrefs, on the symbolcollector side I don't think there's anything to do? ================ 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: > > > > > > 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; ``` 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
