sammccall added a comment.
Thanks, this looks pretty good, I think there are a couple of adjustments...
================
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:
> > > > > > > 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?
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:160
-// Checks whether \p ND is a definition of a TagDecl (class/struct/enum/union)
-// in a header file, in which case clangd would prefer to use ND as a canonical
-// declaration.
+// Checks whether \p ND is a preferred declaration to clangd. Typically this
+// means preferring true declarations over forward declarations.
----------------
"preferred" declaration is just the method name, and "true" declaration doesn't
seem to clarify much.
For this overview paragraph I'd suggest: Checks whether \p ND is a good
candidate to be the *canonical* declaration of its symbol (e.g. a
go-to-declaration target). This overrides the default of using Clang's
canonical declaration, which is the first in the TU.
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:163
+//
+// For C++:
+// If \p ND is a definition of a TagDecl (class/struct/enum/union) in a header,
----------------
I'm not sure the for C++/for ObjC split in the doc makes sense - these special
cases are more to do with the node types than the underlying languages (which
can overlap).
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:186
+//
+// For example, Objective-C classes can have three types of
+// declarations, all with the same USR:
----------------
this isn't really an example, it's the only case. I think it can be moved
inside the function, above the case itself.
"We treat ObjCImplDecl as a definition but not a declaration - the
corresponding ObjcInterfaceDecl is the declaration"
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:371
+ // TU, because in practice they are definitions.
+ if (shouldRecordDeclaration(*ND)) {
+ if (isPreferredDeclaration(*OriginalDecl, Roles))
----------------
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
```
================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:376
+ else
+ BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
+ }
----------------
you're now clobbering BasicSymbol every time, even if it was set and this isn't
a preferred declaration
================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:611
+ Code = R"cpp(
+ @interface Foo
----------------
can we add a case here and in symbolcollector testing the "implicit interface
decl" case? (ObjCInterfaceDecl::isImplicitInterfaceDecl())
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
