usaxena95 marked 2 inline comments as done.
usaxena95 added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:463
+ }
+ // Query the index for references from other files.
+ if (Index && Results.References.size() < Limit) {
----------------
kadircet wrote:
> could we merge this and the code for decls by only populating `Req.IDs` with
> `MacroSID` here and with the symbol id below and by finally making the query
> in the end ? i.e.
>
> ```
> RefsRequest Req;
> Req.Limit = Limit;
> if (macro) {
> handleMainFileMacros;
> Req.IDs.insert(*MacroID);
> } else {
> handleMainFileDecls;
> populateReqIDs(Req);
> }
> handleIndexResults(Req);
> ```
Tried to do something on those lines.
================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1021
TEST(FindReferences, NeedsIndex) {
+ const char *Header = (R"cpp(
----------------
kadircet wrote:
> nit: i don't think there's much benefit in combining refs for macros and
> symbols in a single test. their handling in the code is disjoint, whereas
> this test is not. so it makes the test a little bit harder to read and also
> failures would be harder to reason about. but the test is currently small, so
> up to you whether you want to separate it or not.
Added the test for macros as a separate test.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72395/new/
https://reviews.llvm.org/D72395
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
