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

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added inline comments.


================
Comment at: clangd/index/FileIndex.h:47
+  // The shared_ptr keeps the symbols alive.
+  std::shared_ptr<SymbolIndex> buildMemIndex();
 
----------------
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!


================
Comment at: clangd/index/Index.h:15
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/Any.h"
 #include "llvm/ADT/DenseMap.h"
----------------
nit: no longer needed?


================
Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template <typename Range> DexIndex(Range &&Symbols) {
+    for (auto &&Sym : Symbols)
----------------
sammccall wrote:
> ioeric wrote:
> > sammccall wrote:
> > > 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?
> > > What stops us adding more parameters?
> > I thought this template was added so that we could use it as a drop-in replacement of `MemIndex` (with the same signature) e.g. in `FileIndex`? I might have overthought though.
> > 
> > Thanks for the explanation!
> Sure, Dex and MemIndex had the same constructor signatures as each other before this patch, and they have the same as each other after this patch.
> 
> That makes it convenient to use them interchangeably, but there's no hard requirement (no construction from templates etc).
Sounds good.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D51422