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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:360
+                   dyn_cast<ObjCImplementationDecl>(D)) {
+      // Objective-C implementation should map back to its interface.
+      D = IID->getClassInterface();
----------------
This just describes what the code is doing, say why instead?
// We treat ObjC{Interface,Implementation}Decl as if they were a decl/def pair.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:696
+  )cpp";
+  EXPECT_DECLS("ObjCImplementationDecl", "@interface Implicit");
+
----------------
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...


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



================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:710
+      R"objc(
+        @class Foo;
+        @interface $decl[[Foo]]
----------------
maybe add a comment like // prefer interface definition over forward declaration


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:803
+  };
+  for (const char *Test : Tests) {
+    Annotations T(Test);
----------------
this seems to be copy/pasted from the test above.
Is there a reason this can't be part of the test above?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83501