[PATCH] D83501: [clangd][ObjC] Improve xrefs for protocols and classes

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Ugh, I forgot to hit submit and went on vacation :-\ Really sorry again.

As much as we can simplify/unify the tests that helps, but let's not block on this anymore, up to you.



================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:614
+
+TEST_F(SymbolCollectorTest, ObjCClassExtensions) {
+  Annotations Header(R"(
----------------
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".


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:803
+  };
+  for (const char *Test : Tests) {
+    Annotations T(Test);
----------------
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)


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:838
+
+TEST(LocateSymbol, MultipleDeclsWithSameDefinition) {
+  // Ranges in tests:
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83501/new/

https://reviews.llvm.org/D83501