dgoldman added inline comments.
================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:614
+
+TEST_F(SymbolCollectorTest, ObjCClassExtensions) {
+ Annotations Header(R"(
----------------
sammccall wrote:
> dgoldman wrote:
> > 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?
> Sorry, I don't know what you mean about QName. USR?
>
> Yeah, fine to defer out of this patch. This is just "does find references on
> an @interface find extensions of that interface".
QualifiedName, will follow up later
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:803
+ };
+ for (const char *Test : Tests) {
+ Annotations T(Test);
----------------
sammccall wrote:
> dgoldman wrote:
> > 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?
> I think it would be better to split each decl/def pair into its own testcase,
> and combine with the above (even if it means a bit of duplication between
> test cases)
These tests are odd since one point (^) can trigger multiple decls, so I think
it's worth keeping separate vs. the other tests which assume 1 decl/def pair.
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:838
+
+TEST(LocateSymbol, MultipleDeclsWithSameDefinition) {
+ // Ranges in tests:
----------------
sammccall wrote:
> dgoldman wrote:
> > 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
> > This one is split because you can't annotate one symbol with multiple
> > annotations
>
> Not sure I quite see what you mean here, but `$foo[[$bar[[symbol]]]] should
> work`.
>
> Anyway, fine if you want to leave this one separate. I'd avoid the tests
> array+loop if there's just one.
🤦 Ah thanks, that 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
