[PATCH] D90750: [clangd] Introduce ProjectAwareIndex

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/Config.h:88
 
+inline bool operator<(const Config::ExternalIndexSpec &LHS,
+                      const Config::ExternalIndexSpec &RHS) {
----------------
Maybe we could use DenseMap instead? Defining an order on specs seems semantically iffy.

(Maybe one day we'll get to use a better hashtable and DenseMapInfo won't be so gross)


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:137
+  if (!Gen) {
+    Gen = [](const Config::ExternalIndexSpec &External,
+             AsyncTaskRunner &Tasks) {
----------------
nit: I think this would be clearer as a named function instead of a lambda


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:100
+          });
+          addIndex(std::move(NewIndex));
+          return PlaceHolder;
----------------
kadircet wrote:
> sammccall wrote:
> > from memoize:
> > 
> > > Concurrent calls for the same key may run the
> > > computation multiple times, but each call will return the same result.
> > 
> > so this side-effect may run multiple times too, resulting in multiple index copies getting leaked. This seems like it may be bad in the case of file indexes.
> > 
> > Why can't the memoize map own the indexes?
> > This would still result in loading the index multiple times and throwing things away, hmm. Not sure how best to resolve this.
> > so this side-effect may run multiple times too, resulting in multiple index copies getting leaked. This seems like it may be bad in the case of file indexes.
> 
> The only way i can see this happening is by synchronizing calls to `getIndex`.
> 
> > Why can't the memoize map own the indexes?
> 
> Main problem is, it returns a copy to the stored elements, rather than a reference to them. Which might be necessary in general to guarantee thread-safety. Since we can't copy unique_ptrs, I had to store them elsewhere and only keep pointers to it within the memoize map.
> 
> 
> So both things considered, maybe we should just give up on memoize and have our own thread-safe cache here?
Yeah given that we know the compute function is going to be fast, we can just run it under the lock. So let's just use a big fat mutex.


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.h:21
+/// A functor to create an index for an external index specification.
+using IndexGenerator = std::function<std::unique_ptr<SymbolIndex>(
+    const Config::ExternalIndexSpec &, AsyncTaskRunner &)>;
----------------
nit: "factory" is more common than "generator" for this purpose, probably


================
Comment at: clang-tools-extra/clangd/index/ProjectAware.h:25
+/// Returns an index that answers queries using external indices. IndexGenerator
+/// can be used to customize how to generate an index from an external source.
+std::unique_ptr<SymbolIndex> createProjectAwareIndex(IndexGenerator = nullptr);
----------------
Mention the default?

The default implementation loads the index asynchronously on the AsyncTaskRunner.
The index will appear empty until loaded.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90750/new/

https://reviews.llvm.org/D90750