[PATCH] D50958: [clangd] Implement findReferences function

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

OK thanks, I'll steal this one.



================
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.
----------------
hokein wrote:
> 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).
Agree, this sounds like the right place to start.


================
Comment at: unittests/clangd/XRefsTests.cpp:1193
+  )";
+  MockIndex Index;
+  for (const char *Test : Tests) {
----------------
hokein wrote:
> 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.
ah, that makes sense. But we should have some actual tests that use the index content too!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50958