Page MenuHomePhabricator

[clangd] Add an interface that finds symbol by SymbolID in SymbolIndex.
ClosedPublic

Authored by ioeric on Mar 9 2018, 7:34 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

ioeric created this revision.Mar 9 2018, 7:34 AM

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

clangd/index/Index.h
268 ↗(On Diff #137751)

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!)

268 ↗(On Diff #137751)

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.

268 ↗(On Diff #137751)

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?

clangd/index/Merge.cpp
58 ↗(On Diff #137751)

(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)

unittests/clangd/IndexTests.cpp
93 ↗(On Diff #137751)

Symbol;Scope is already e.g. "ns::", this shouldn't be needed.

112 ↗(On Diff #137751)

check the return value

ioeric updated this revision to Diff 138204.Mar 13 2018, 8:52 AM
ioeric marked 6 inline comments as done.
  • - Addressed review comments.

Thanks for the review!

clangd/index/Index.h
268 ↗(On Diff #137751)

Makes sense. I wasn't really sure about getSymbol and wanted your thought. Going with lookup.

unittests/clangd/IndexTests.cpp
93 ↗(On Diff #137751)

This wasn't true in the tests. Fixed.

112 ↗(On Diff #137751)

This was intended. If no symbol was found, Res would be empty by default. No longer relevant in the new revision.

sammccall accepted this revision.Mar 13 2018, 10:20 AM
sammccall added inline comments.
clangd/index/Index.h
253 ↗(On Diff #138204)

nit: DenseSet? we already have the traits

274 ↗(On Diff #138204)

is "at least one matched symbol was found" so useful that we need to provide it in a second way (rather than just invoking the callback at least once)?

Having both fuzzyFind and lookup return bool (but with very different semantics) seems confusing, and I suspect void would be fine here.

clangd/index/Merge.cpp
67 ↗(On Diff #138204)

this could just be callback(S)

69 ↗(On Diff #138204)

This could also just be callback(mergeSymbol(...)), if we keep track of the IDs we've emitted.
This way the slab would only have symbols from the dynamic index.

This could be just:

  • before the static lookup, make a copy RemainingIDs = Req.IDs
  • in the static callback, remove the id from RemainingIDs
  • after the static lookup, loop through RemainingIDs, look up symbol in dynamic slab, emit

This avoids:

  • copying the static index results (which tend to be more numerous)
  • build() on the slab builder, which isn't cheap
unittests/clangd/CodeCompleteTests.cpp
693 ↗(On Diff #138204)

why comment out the param names here? can't we just either include or omit them?

This revision is now accepted and ready to land.Mar 13 2018, 10:20 AM
ioeric updated this revision to Diff 138313.Mar 14 2018, 2:48 AM
ioeric marked 5 inline comments as done.
  • Addressed review comments.
clangd/index/Merge.cpp
69 ↗(On Diff #138204)

Good idea. Thanks!

This revision was automatically updated to reflect the committed changes.