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

[Quentin Chateau via Phabricator via cfe-commits]

qchateau added inline comments.


================
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 &&
----------------
nridge wrote:
> 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.
I've used tried this patch with the whole llvm project indexed. If I remember correctly it's something in the like of 100MB, around 5-10% même usage of clangd. As such it's not very high but it's definitely not negligible, especially for a feature that's not used very often.


================
Comment at: clang-tools-extra/clangd/index/Index.h:85
 
+struct RefersToResult {
+  /// The source location where the symbol is named.
----------------
nridge wrote:
> 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.)
I indeed believe that I'd use more memory than it would save, but I did not try it.
Anyway I think the difference would be minimal, and I'd say we should choose what gives the "best" 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;
----------------
nridge wrote:
> Perhaps we should have a distinct request structure, for future extensibility?
Sure, that makes sense


================
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) {
----------------
nridge wrote:
> Looping over all refs seems expensive even for `MemIndex`. Perhaps we should have a reverse lookup table similar to `RevRefs` here as well?
That's pretty fast even on my laptop IIRC, and only used for outgoing calls atm. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93829