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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

OK, I think this is good to go again.
(A couple of seemingly-unrelated fixes to SymbolOccurrenceKind that I ran into in tests)



================
Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr<SymbolIndex> buildMemIndex();
 
----------------
ioeric wrote:
> kbobyrev wrote:
> > 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?
> Provide one interface for each index implementation sounds good to me. Thanks for the clarification!
I'm not really sure what problem templates solve there?
If a caller wants a Dex index, we can just add a `buildDexIndex()` function.
Templating and requiring these to always have the same constructor parameters seems fragile and unneccesary unless the dependency from FileIndex->Dex is really bad for some reason.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422