dgoldman added a comment.
In D83501#2173534 <https://reviews.llvm.org/D83501#2173534>, @sammccall wrote:
> (Sorry this has been pending a while - I think it's basically there. Only
> things we really need to address to land this is have a consistent view of
> what the canonical decl is for the no-@interface case, and avoid too much
> duplication of mechanisms in the tests)
No problem, thanks for the review
================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:696
+ )cpp";
+ EXPECT_DECLS("ObjCImplementationDecl", "@interface Implicit");
+
----------------
sammccall wrote:
> Hmm, do we want to use the @interface or @implementation for this case? The
> interface is implicit but probably still has a valid location.
> Currently symbolcollector and findtarget do different things...
Good catch, don't think this is a common case but yeah I think the impl makes
more sense then. swapped over
================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:614
+
+TEST_F(SymbolCollectorTest, ObjCClassExtensions) {
+ Annotations Header(R"(
----------------
sammccall wrote:
> dgoldman wrote:
> > Here's the ClassExtension that I was talking about.
> >
> > Ideally we can map each
> >
> > `Cat ()` --> `@implementation Cat` like I did in XRefs
> >
> > But as you said the `Cat ()` could be in a different file and I think it
> > has a different USR.
> >
> > See also
> > https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/CustomizingExistingClasses/CustomizingExistingClasses.html#//apple_ref/doc/uid/TP40011210-CH6-SW3
> > Ideally we can map each
> > Cat () --> @implementation Cat like I did in XRefs
>
> I'm not sure there's anything that would ideally be done differently here.
> The logic in xrefs is a special "go-to-definition" action - there's some
> ambiguity about what's being *targeted* by the user. But here there's no
> targeting going on, and there's no ambiguity about what's being *declared*.
>
> The thing to test would be that we're emitting *refs* from `@interface
> [[Cat]] ()` to catdecl.
>
Hmm, it looks like at the moment it either shares the same QName or doesn't
have one. This might be good to look into a follow up patch?
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:803
+ };
+ for (const char *Test : Tests) {
+ Annotations T(Test);
----------------
sammccall wrote:
> this seems to be copy/pasted from the test above.
> Is there a reason this can't be part of the test above?
I could merge them but I figured it would be better to separate tests with
multi def/decls from those with just one. WDYT?
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:838
+
+TEST(LocateSymbol, MultipleDeclsWithSameDefinition) {
+ // Ranges in tests:
----------------
sammccall wrote:
> and again here
>
> Desire to split these tables up into named tests is something we want to
> address somehow, but we don't have a good answer right now and it's important
> for maintenance that the logic/annotation conventions don't diverge across
> different tests that could be the same.
This one is split because you can't annotate one symbol with multiple
annotations. I can instead make this a regular non generic test like the
following, WDYT?
@interface $interfacedecl[[Cat]]
@end
@interface $classextensiondecl[[Ca^t]] ()
- (void)meow;
@end
@implementation $implementationdecl[[Cat]]
- (void)meow {}
@end
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
