[PATCH] D50958: [clangd] Implement findReferences function

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In https://reviews.llvm.org/D50958#1222249, @sammccall wrote:

> 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!


Thanks, feel free to pick it up. The comments make sense to me. Just let you know I have one more xref patch: https://reviews.llvm.org/D50896.



================
Comment at: clangd/XRefs.cpp:333
+
+    DeclOccurrences[D].emplace_back(FileLoc, Roles);
+    return true;
----------------
sammccall wrote:
> why is this a map keyed by D? the keys are never used, the result is always flattened into a vector.
Using map can group results by the Decl, but it doesn't matter here.


================
Comment at: clangd/XRefs.cpp:378
+    }
+    // Deduplicate results.
+    std::sort(Occurrences.begin(), Occurrences.end());
----------------
sammccall wrote:
> out of curiosity: how do we actually get duplicates?
Some AST nodes will be visited more than once, so we might have duplicated locations.


================
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.
----------------
sammccall wrote:
> we can check isExternallyVisible, I think?
> maybe it's not worth it, but the comment as it stands doesn't seem accurate
I'm not sure whether `isExternallyVisible` suits our cases. For example, if a header defines a `static const int MAX`, and the header is widely included in the project, we want to query it in the index, but the linkage of the symbol `MAX` is internal.

So I'd prefer to be `conservative` here (only for function-local symbols).


================
Comment at: unittests/clangd/XRefsTests.cpp:1193
+  )";
+  MockIndex Index;
+  for (const char *Test : Tests) {
----------------
sammccall wrote:
> 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?
Yes, we can. But the test here is **only** to verify whether the index has been queried. The results from index are not important.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958