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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
