[PATCH] D51422: [clangd] Factor out the data-swapping functionality from MemIndex/DexIndex.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr<SymbolIndex> buildMemIndex();
 
----------------
ioeric wrote:
> Maybe avoid hardcoding the index name, so that we could potentially switch to use a different index implementation?
> 
> We might also want to allow user to specify different index implementations for file index e.g. main file dynamic index might prefer MemIndex while Dex might be a better choice for the preamble index. 
I don't think "returns an index of unspecified implementation" is the best contract. MemIndex and DexIndex will both continue to exist, and they have different performance characteristics (roughly fast build vs fast query). So it should be up to the caller, no?


================
Comment at: clangd/index/MemIndex.h:30
+  /// Builds an index from a slab. The shared_ptr manages the slab's lifetime.
+  static std::shared_ptr<SymbolIndex> build(SymbolSlab Slab);
 
----------------
ioeric wrote:
> sammccall wrote:
> > ioeric wrote:
> > > (It's a bit unfortunate that this has to return `shared_ptr` now)
> > Since some of the resources it owns has a shared lifetime, this is really just reflecting reality I think. Whether that's visible or invisible seems like a wash to me.
> This is only true when this is used with `SwapIndex` right? For example, a static Dex/Mem index would probbaly have `unique_ptr` ownership.
Dex and MemIndex don't own the symbols.

If S is a SymbolSlab:
 - `MemIndex(S)` (or `make_unique<MemIndex>(S)`) is a MemIndex that doesn't know about the storage and doesn't own the symbols
 - `MemIndex::build(move(S))` is a `shared_ptr<MemIndex>` that owns the storage. It's the `shared_ptr`, not the `MemIndex`, that manages the storage.

So for a static index, you could either hand off the ownership of the slab by calling build(), or keep it yourself and call the constructor. The former seems more convenient in ClangdMain.

We could write this differently, e.g. as `std::shared_ptr<SymbolIndex> = tieStorageToIndex(std::move(S), make_unique<MemIndex>(S));`. The API here (`MemIndex::build()`) is intended to make common use cases easy, and keep it fairly similar to before for migration purposes. Is it too confusing?


================
Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template <typename Range> DexIndex(Range &&Symbols) {
+    for (auto &&Sym : Symbols)
----------------
ioeric wrote:
> sammccall wrote:
> > ioeric wrote:
> > > Why is this constructor needed? I think this could restrict the flexibility of DexIndex initialization, in case we need to pass in extra parameters here.
> > I'm not sure exactly what you mean here.
> > 
> > We need a way to specify the symbols to be indexed.
> > Because these are now immutable, doing that in the constructor if possible is best.
> > 
> > Previously this was a vector<const Symbol*>, but that sometimes required us to construct that big vector, dereference all those pointers, and throw away the vector. This signature is strictly more general (if you have a vector of pointers, you can pass `make_pointee_range<const Symbol>`)
> > 
> > > in case we need to pass in extra parameters here.
> > What stops us adding more parameters?
> > What stops us adding more parameters?
> I thought this template was added so that we could use it as a drop-in replacement of `MemIndex` (with the same signature) e.g. in `FileIndex`? I might have overthought though.
> 
> Thanks for the explanation!
Sure, Dex and MemIndex had the same constructor signatures as each other before this patch, and they have the same as each other after this patch.

That makes it convenient to use them interchangeably, but there's no hard requirement (no construction from templates etc).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422