[PATCH] D44305: [clangd] Add an interface that finds symbol by SymbolID in SymbolIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this! Some quibbles about the interface, but this makes index useful for lots more features.



================
Comment at: clangd/index/Index.h:268
+  virtual bool
+  getSymbol(const SymbolID &ID,
+            llvm::function_ref<void(const Symbol &)> Callback) const = 0;
----------------
Can we make this a bulk operation (take an arrayref<SymbolID> or similar?)

There are use cases like augmenting sema-returned results with info from the index where we want a bunch at once. In practice a single bulk operation will be much nicer for an rpc-based index to implement than a single lookup issued many times in parallel.

(The callback interface is really nice here, because the underlying RPC can be streaming!)


================
Comment at: clangd/index/Index.h:268
+  virtual bool
+  getSymbol(const SymbolID &ID,
+            llvm::function_ref<void(const Symbol &)> Callback) const = 0;
----------------
sammccall wrote:
> Can we make this a bulk operation (take an arrayref<SymbolID> or similar?)
> 
> There are use cases like augmenting sema-returned results with info from the index where we want a bunch at once. In practice a single bulk operation will be much nicer for an rpc-based index to implement than a single lookup issued many times in parallel.
> 
> (The callback interface is really nice here, because the underlying RPC can be streaming!)
For extensibility and uniformity with FuzzyFind, we should consider adding a struct around the parameters.

At least one option seems likely to be added here: retrieving the full ("detail") symbol vs the basic symbol (particularly for bulk actions).
Others are less obvious, but could include something like "chase pointers" so that if returning a typedef, the target of the typedef would also be looked up and returned.



================
Comment at: clangd/index/Index.h:268
+  virtual bool
+  getSymbol(const SymbolID &ID,
+            llvm::function_ref<void(const Symbol &)> Callback) const = 0;
----------------
sammccall wrote:
> sammccall wrote:
> > Can we make this a bulk operation (take an arrayref<SymbolID> or similar?)
> > 
> > There are use cases like augmenting sema-returned results with info from the index where we want a bunch at once. In practice a single bulk operation will be much nicer for an rpc-based index to implement than a single lookup issued many times in parallel.
> > 
> > (The callback interface is really nice here, because the underlying RPC can be streaming!)
> For extensibility and uniformity with FuzzyFind, we should consider adding a struct around the parameters.
> 
> At least one option seems likely to be added here: retrieving the full ("detail") symbol vs the basic symbol (particularly for bulk actions).
> Others are less obvious, but could include something like "chase pointers" so that if returning a typedef, the target of the typedef would also be looked up and returned.
> 
`getSymbol` isn't a bad name, but it's a bit hard to talk about without ambiguity because "get" is so overloaded and everything deals with "symbols".. (e.g. "this method gets a symbol as a parameter..."). It's also awkward to use as a noun, which is common with RPCs.

`lookup` or `fetch` would be specific enough to avoid this. (Dropping "symbol" from the method name because it's present in the interface name). WDYT?


================
Comment at: clangd/index/Merge.cpp:58
+            llvm::function_ref<void(const Symbol &)> Callback) const override {
+    SymbolSlab::Builder B; // Used to store the symbol from dynamic index.
+    if (!Dynamic->getSymbol(ID, [&](const Symbol &S) { B.insert(S); }))
----------------
(Seems like this is avoidable when looking up a single key, by nesting the second call in the callback. But with a batch operation the slab is probably necessary)


================
Comment at: unittests/clangd/IndexTests.cpp:93
+std::string getQualifiedName(const Symbol &Sym) {
+  return (Sym.Scope + (Sym.Scope.empty() ? "" : "::") + Sym.Name).str();
+}
----------------
Symbol;Scope is already e.g. "ns::", this shouldn't be needed.


================
Comment at: unittests/clangd/IndexTests.cpp:112
+  std::string Res = "";
+  I.getSymbol(ID, [&](const Symbol &Sym) { Res = getQualifiedName(Sym); });
+  return Res;
----------------
check the return value


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44305