ioeric added inline comments.
================
Comment at: clangd/index/MemIndex.h:30
+ /// Builds an index from a slab. The shared_ptr manages the slab's lifetime.
+ static std::shared_ptr<SymbolIndex> build(SymbolSlab Slab);
----------------
sammccall wrote:
> ioeric wrote:
> > (It's a bit unfortunate that this has to return `shared_ptr` now)
> Since some of the resources it owns has a shared lifetime, this is really
> just reflecting reality I think. Whether that's visible or invisible seems
> like a wash to me.
This is only true when this is used with `SwapIndex` right? For example, a
static Dex/Mem index would probbaly have `unique_ptr` ownership.
================
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:
> > 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!
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
