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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
