kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks, lgtm!
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:496
+ std::condition_variable CV;
+ std::atomic<bool> ShouldStop = {false}; // Must notify CV after writing.
+ std::deque<CDBLookupResult> Queue;
----------------
sammccall wrote:
> kadircet wrote:
> > nit: we already hold the lock within the loop while reading. it is nice
> > that we no longer need to acquire the lock during destructor, but is it
> > worth the extra mental load that comes with memory orderings? (also IIRC,
> > default load/store behaviors are already acquire-release)
> Whoops, I split this patch in half and now this makes no sense.
>
> The point is rather that we should interrupt the "evaluate the config for
> every file" loop (which will be part of process()) if ShouldStop is set. So
> we check this atomic inside that loop, without acquiring the lock.
>
> (In practice maybe this is always "fast enough" that we should just run the
> whole loop, enqueue all the background indexing stuff, and then shut down
> afterwards?)
>
> > (also IIRC, default load/store behaviors are already acquire-release)
>
> Default is sequentially consistent, which is almost always more than we need.
>
> ---
>
> I've added a check to the existing loop (which probes directory caches), less
> necessary but shows the idea. If you prefer to drop this entirely that's OK
> too.
> The point is rather that we should interrupt the "evaluate the config for
> every file" loop (which will be part of process()) if ShouldStop is set. So
> we check this atomic inside that loop, without acquiring the lock.
ah okay now it makes sense.
> I've added a check to the existing loop (which probes directory caches), less
> necessary but shows the idea. If you prefer to drop this entirely that's OK
> too.
i suppose we could end up blocking the shutdown for quite some time in
pathological cases due to disk io. so this looks like a good tradeoff.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94606/new/
https://reviews.llvm.org/D94606
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
