sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:279
std::deque<Request> Requests; /* GUARDED_BY(Mutex) */
+ bool RunningTask = false; /* GUARDED_BY(Mutex) */
mutable std::condition_variable RequestsCV;
----------------
I'd consider making this an `llvm::Optional<Request>` and using it as storage
too. Even though it won't be accessed directly outside the main loop, I think
it makes it easier to reason about the code: it emphasizes the parallel with
`Requests` and minimizes the spooky-flags-at-a-distance effect.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:612
// If there were no writes in the queue, the preamble is ready now.
if (LastUpdate == Requests.rend()) {
Lock.unlock();
----------------
what if the currently-running task is a write?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:721
{
std::unique_lock<std::mutex> Lock(Mutex);
for (auto Wait = scheduleLocked(); !Wait.expired();
----------------
maybe assert there's no running task here?
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:376
Notification Start;
- updateWithDiags(S, Path, "a", WantDiagnostics::Yes, [&](std::vector<Diag>) {
- ++Builds;
- Start.wait();
- });
+ updateWithDiags(S, Path, "b", WantDiagnostics::Auto,
+ [&](std::vector<Diag>) { ++Builds; });
----------------
does this deliberately match "b" below? if not, change back to something unique?
If so, probably needs a comment.
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:379
+ ASSERT_TRUE(S.blockUntilIdle(timeoutSeconds(10)));
+ S.runWithAST(
+ "invalidatable-but-running", Path,
----------------
This looks racy to me. What guarantees that the worker thread pulls this item
off the queue and into the "running" slot before the second updateWithDiags?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75927/new/
https://reviews.llvm.org/D75927
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
