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