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