nridge added a comment.
Thanks for working on this!
I haven't looked at the whole patch in detail, but I looked at some parts
(mainly the `SymbolIndex` API and what in-memory structures we store to
implement it) and wrote a few thoughts.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1843
+ auto Kind = Callee.SymInfo.Kind;
+ if (Kind != SK::Function && Kind != SK::InstanceMethod &&
+ Kind != SK::ClassMethod && Kind != SK::StaticMethod &&
----------------
If memory usage of `Dex::RevRefs` becomes a concern, we could consider only
storing the references that would pass this filter in the first place. That
would trade off time spent building the reverse lookup (we'd have to do symbol
lookups or keep around extra state to track the symbol kinds) for space savings.
================
Comment at: clang-tools-extra/clangd/index/Index.h:85
+struct RefersToResult {
+ /// The source location where the symbol is named.
----------------
As the protocol wants the outgoing calls grouped by symbol, we could consider
storing them (and having the `SymbolIndex` API expose them) in that way.
The main motivation would be space savings on the storage side
(`Dex::RevRefs`), as in the case of multiple calls to the same function we only
need to store the target `SymbolID` once.
However, it may not be worth doing this, as having a large number of outgoing
calls to the same target inside a given function is likely to be rare, and
vectors have their own overhead. (It's also not clear to what extent the
details of the LSP protocol should dictate the shape of the `SymbolIndex` API.)
================
Comment at: clang-tools-extra/clangd/index/Index.h:133
+ virtual bool
+ refersTo(const RefsRequest &Req,
+ llvm::function_ref<void(const RefersToResult &)> Callback) const =
0;
----------------
Perhaps we should have a distinct request structure, for future extensibility?
================
Comment at: clang-tools-extra/clangd/index/MemIndex.cpp:97
+ Req.Limit.getValueOr(std::numeric_limits<uint32_t>::max());
+ for (const auto &Pair : Refs) {
+ for (const auto &R : Pair.second) {
----------------
Looping over all refs seems expensive even for `MemIndex`. Perhaps we should
have a reverse lookup table similar to `RevRefs` here as well?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93829/new/
https://reviews.llvm.org/D93829
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
