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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

Thanks for the review!



================
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:
> > 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?
Makes sense. I wasn't really sure about `getSymbol` and wanted your thought. Going with `lookup`.


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


================
Comment at: unittests/clangd/IndexTests.cpp:112
+  std::string Res = "";
+  I.getSymbol(ID, [&](const Symbol &Sym) { Res = getQualifiedName(Sym); });
+  return Res;
----------------
sammccall wrote:
> check the return value
This was intended. If no symbol was found, `Res` would be empty by default. No longer relevant in the new revision.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44305