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