[PATCH] D93829: [clangd] Support outgoing calls in call hierarchy

[Nathan Ridge via Phabricator via cfe-commits]

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