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

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
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);
 
----------------
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.


================
Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template <typename Range> DexIndex(Range &&Symbols) {
+    for (auto &&Sym : Symbols)
----------------
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!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422