sammccall added a comment.
This looks really sensible to me! Nice you managed to reuse Memoize, there's
very little ugly caching/locking left.
================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:81
+ return IndexForProject.get(
+ External.Location, [Server = External.Location,
+ MP = llvm::StringRef(External.MountPoint), this] {
----------------
I think the cache key needs a bit more consideration.
It's unlikely that a filename will collide with a host/port, but they are
currently sitting in the same namespace.
More worryingly, the mountpoint is used in the lambda body but not in the cache
key - are we sure this is safe?
The clearest way to enforce this is to make index creation use the cache key
only, and not other data from the config. Then you have to include what you're
going to use.
================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:81
+ return IndexForProject.get(
+ External.Location, [Server = External.Location,
+ MP = llvm::StringRef(External.MountPoint), this] {
----------------
sammccall wrote:
> I think the cache key needs a bit more consideration.
>
> It's unlikely that a filename will collide with a host/port, but they are
> currently sitting in the same namespace.
>
> More worryingly, the mountpoint is used in the lambda body but not in the
> cache key - are we sure this is safe?
>
> The clearest way to enforce this is to make index creation use the cache key
> only, and not other data from the config. Then you have to include what
> you're going to use.
as discussed offline, allowing the config -> cache key and cache key -> index
operations to be injectable is the key to testing I think
================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:100
+ });
+ addIndex(std::move(NewIndex));
+ return PlaceHolder;
----------------
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.
================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:119
+ SymbolIndex *Result = nullptr;
+ auto PushIndex = [this, &Result](SymbolIndex *Idx) {
+ if (!Result) {
----------------
consider moving this to `unique_ptr<MergedIndex>
MergedIndex::create(ArrayRef<unique_ptr<SymbolIndex>>)` or so?
Not sure if this is something we'd want to use from ClangdMain...
================
Comment at: clang-tools-extra/clangd/index/ProjectAware.cpp:128
+ std::lock_guard<std::mutex> Lock(Mu);
+ for (const auto &Idx : IndexStorage) {
+ if (Idx.get() == Primary)
----------------
with the helper described above, the Primary ordering could be maybe better
expressed as std::stable_partition
================
Comment at: clang-tools-extra/clangd/index/ProjectAware.h:30
+
+class ProjectAwareIndex : public SymbolIndex {
+public:
----------------
Is there a reason this class needs to be public?
I think we can expose factories returning SymbolIndex only
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
