[PATCH] D72395: [clangd] Publish xref for macros from Index and AST.

[UTKARSH SAXENA via Phabricator via cfe-commits]

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