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