[PATCH] D99975: [clangd][ObjC] Improve support for class properties

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.
Herald added a subscriber: cfe-commits.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:309-313
+        // FIXME: visiting this here allows us to hover on UIColor in
+        // `UIColor.blackColor` but then `blackColor` no longer refers to the
+        // method.
+        // if (OPRE->isClassReceiver())
+        //   Outer.add(OPRE->getClassReceiver(), Flags);
----------------
dgoldman wrote:
> Is there a good way to handle this case where the expression can refer to multiple decls?
TL;DR: yes, fix the AST :-)

The design assumption is that an AST node (specifically: the tokens owned by exactly that AST node and not a descendant of it) refers to one thing. I don't see an easy way to hack around this.

Expressions like this are supposed to be split into multiple AST nodes. But ObjC seems to be missing AST nodes in a bunch of places (Protocol lists is another).

e.g. in C++ equivalent DeclRefExpr `UIColor::blackColor` there'd be both a NestedNameSpecifier for `UIColor::` and a TagTypeLoc for `UIColor`. And objc `[Test foo]` has an `ObjCInterfaceTypeLoc` for `Test`.

---

I'm not sure this is the right place for this comment, the problem isn't what VisitObjCPropertyRefExpr is doing, the issue is that the caller is passing the wrong kind of node to findTarget() because there's no right one.


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:768
     void VisitObjCPropertyRefExpr(const ObjCPropertyRefExpr *E) {
+      if (E->isClassReceiver()) {
+        Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
----------------
comment here:
// There's no contained TypeLoc node for the receiver type.


================
Comment at: clang-tools-extra/clangd/unittests/FindTargetTests.cpp:1040
-      {
-          // Simple expressions.
-          {R"cpp(
----------------
As in the other patch, we should keep these large unrelated reformattings out of substantive patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99975