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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev accepted this revision.
kbobyrev added a comment.
This revision is now accepted and ready to land.

Other than a hard-coded `buildMemIndex()` in `FileIndex`, I don't have any concerns. That's just a tiny piece, if @ioeric doesn't have any concerns about that too, I think it's good to land.

The code should look cleaner in multiple places now, thanks for the patch!



================
Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr<SymbolIndex> buildMemIndex();
 
----------------
sammccall wrote:
> 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?
We could make the index type a template parameter to allow users decide which one they want (also, this could be have a default value, e.g. `MemIndex` and later `DexIndex`). Would that be a viable solution?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422