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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
