[PATCH] D50958: [clangd] Implement findReferences function

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Looking forward to using this!

Unless you object, I'd like to address the remaining comments (and get a review), so you can make the most of your leave!



================
Comment at: clangd/XRefs.cpp:333
+
+    DeclOccurrences[D].emplace_back(FileLoc, Roles);
+    return true;
----------------
why is this a map keyed by D? the keys are never used, the result is always flattened into a vector.


================
Comment at: clangd/XRefs.cpp:361
+/// Find symbol occurrences that a given whilelist of declarations refers to.
+class OccurrencesFinder : public OccurrenceCollector {
+public:
----------------
why is this a subclass rather than just using OccurrenceCollector and putting the finish() code in findReferences()?


================
Comment at: clangd/XRefs.cpp:378
+    }
+    // Deduplicate results.
+    std::sort(Occurrences.begin(), Occurrences.end());
----------------
out of curiosity: how do we actually get duplicates?


================
Comment at: clangd/XRefs.cpp:388
+
+/// Finds document highlights that a given whitelist of declarations refers to.
+class DocumentHighlightsFinder : public OccurrenceCollector {
----------------
as above, why does this need to be a subclass


================
Comment at: clangd/XRefs.cpp:718
+      getBeginningOfIdentifier(AST, Pos, SourceMgr.getMainFileID());
+  // Identified symbols at a specific position.
+  auto Symbols = getSymbolAtPosition(AST, SourceLocationBeg);
----------------
(comment just echoes code)


================
Comment at: clangd/XRefs.cpp:721
+
+  // For local symbols (e.g. symbols that are only visiable in main file,
+  // symbols defined in function body), we can get complete references by
----------------
visiable -> visible


================
Comment at: clangd/XRefs.cpp:724
+  // traversing the AST, and we don't want to make unnecessary queries to the
+  // index. Howerver, we don't have a reliable way to distinguish file-local
+  // symbols. We conservatively consider function-local symbols.
----------------
we can check isExternallyVisible, I think?
maybe it's not worth it, but the comment as it stands doesn't seem accurate


================
Comment at: unittests/clangd/XRefsTests.cpp:1193
+  )";
+  MockIndex Index;
+  for (const char *Test : Tests) {
----------------
can we use a simple real implementation `TU.index()` instead of mocking here?
I think this is an artifact of the fact that TestTU doesn't actually expose occurrences in the index, can we fix that?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958