[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/Index.h:416
+  // until the call returns (even if reset() is called).
+  bool fuzzyFind(const FuzzyFindRequest &,
+                 llvm::function_ref<void(const Symbol &)>) const override;
----------------
kbobyrev wrote:
> Do we want these functions to be `final`? Since `SwapIndex` is a wrapper around an immutable index structure, I believe it would be unlikely that anything would derive from it.
Maybe unlikely but I don't see a strong reason to enforce one way or the other. (We rarely use final).

One example of where we'll do it: FileIndex inherits SwapIndex, but it should override `estimateMemoryUsage` once that's fixed to incorporate backing symbols.


================
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:
> (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.


================
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:
> 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?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422