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