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