[PATCH] D97617: [clangd] ObjC fixes for semantic highlighting and xref highlights

[David Goldman via Phabricator via cfe-commits]

dgoldman added a comment.

In D97617#2592424 <https://reviews.llvm.org/D97617#2592424>, @sammccall wrote:

> (not really sure why this suddenly seemed important to me, but if you don't have semantic highlighting on, you're missing out!)

Thanks!



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:657
+    void VisitObjCInterfaceDecl(const ObjCInterfaceDecl *OID) {
+      if (OID->hasDefinition())
+        visitProtocolList(OID->protocols(), OID->protocol_locs());
----------------
Should this be isThisDeclarationADefinition()? Is it even needed (will the protocol list just be empty otherwise)?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:659
+        visitProtocolList(OID->protocols(), OID->protocol_locs());
+      Base::VisitObjCInterfaceDecl(OID); // Visit the interface itself.
+    }
----------------
Hmm, for what? might be better to say why/what it does


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:668
+                                  /*IsDecl=*/false,
+                                  {OCD->getClassInterface()}});
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
----------------
IIRC I think it's possible that these could be NULL if the code was invalid, is this NULL safe?


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:687-689
+      if (OPD->hasDefinition())
+        visitProtocolList(OPD->protocols(), OPD->protocol_locs());
+      Base::VisitObjCProtocolDecl(OPD); // Visit the protocol itself.
----------------
Same comments here as the interface code above


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:774
+      Refs.push_back(ReferenceLoc{NestedNameSpecifierLoc(),
+                                  E->getSelectorStartLoc(),
+                                  /*IsDecl=*/false, Targets});
----------------
What is this location used for? I guess it's not possible to have multiple Locs here like we have for a selector and you specifically handle for semantic highlighting below?


================
Comment at: clang-tools-extra/clangd/XRefs.cpp:927-930
+      // Possible it was something else referencing an ObjCMethodDecl.
+      // If the first token doesn't match, assume our guess was wrong.
+      if (!Locs.empty() && Locs.front() != Loc)
+        Locs.clear();
----------------
Hmm, is this intentional? Not sure what else could reference it besides maybe a property?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97617