[PATCH] D72746: [clangd] Add a flag for implicit references in the Index

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

thanks, looks good, a few nits.

before landing the patch, could you please run the clangd-indexer on the llvm project and measure the before/after indexing time? to make sure we don't introduce a big latency.
you can run the command like  `time ./bin/clangd-indexer -format=binary -executor=all-TUs . > static-index.idx`



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:588
     for (const auto &LocAndRole : IDAndRefs.second)
       CollectRef(IDAndRefs.first, LocAndRole);
   // Populate Refs slab from DeclRefs.
----------------
macro is tricky, macro references are not marked `spelled`, it is OK for now as we are not interested in them, but I think most of time they are spelled in the source code. Maybe add a comment?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:603
+        // corresponding NamedDecl is. If it isn't, mark this reference as
+        // implicit. An example of implicit reference (implicit = !spelled)
+        // would be a macro expansion.
----------------
nit: the comment seems stale now.


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:723
+  AllRanges.insert(end(AllRanges), begin(ImplicitRanges), end(ImplicitRanges));
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+                                  HaveRanges(AllRanges))));
----------------
nit: instead of checking all refs, I'd check implicit references explicitly, similar to the SpelledRefs below (just add a UnspelledSlab).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72746/new/

https://reviews.llvm.org/D72746