sammccall added inline comments.

================
Comment at: clangd/index/Index.h:416
+  // until the call returns (even if reset() is called).
+  bool fuzzyFind(const FuzzyFindRequest &,
+                 llvm::function_ref<void(const Symbol &)>) const override;
----------------
kbobyrev wrote:
> Do we want these functions to be `final`? Since `SwapIndex` is a wrapper 
> around an immutable index structure, I believe it would be unlikely that 
> anything would derive from it.
Maybe unlikely but I don't see a strong reason to enforce one way or the other. 
(We rarely use final).

One example of where we'll do it: FileIndex inherits SwapIndex, but it should 
override `estimateMemoryUsage` once that's fixed to incorporate backing symbols.


================
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);
 
----------------
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.


================
Comment at: clangd/index/dex/DexIndex.h:42
+  // All symbols must outlive this index.
+  template <typename Range> DexIndex(Range &&Symbols) {
+    for (auto &&Sym : Symbols)
----------------
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?


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

Reply via email to