sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1138
+ if (!wait(Lock, RequestsCV, Timeout,
+ [&] { return Requests.empty() && !CurrentRequest; }))
+ return false;
----------------
I'd consider pulling out an IsIdle lambda (checking for all 3 conditions) and
using it both here and at the bottom.
PreambleRequests will never be (solely) full here, and Requests will always be
empty at the bottom, but it's harmless and I think easier to read.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1142
+ // Now that ASTWorker processed all requests, ensure PreamblePeer has served
+ // all update requests.
+ if (!PreamblePeer.blockUntilIdle(Timeout))
----------------
Maybe explicitly: "This might create new PreambleRequests for the ASTWorker."
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1146
+ Lock.lock();
+ assert(Requests.empty() && "Received new requests during blockUntilIdle");
+ // Finally make sure ASTWorker has processed all of the preamble updates.
----------------
sammccall wrote:
> um... isn't that allowed?
> If new traffic means the queue doesn't go idle, I think we're supposed to
> return false, rather than crash.
OK, I somehowe forgot that ASTWorker isn't threadsafe so we only have the
current thread and the worker thread to worry about.
Suggest a slightly expanded message like: No new normal tasks can be scheduled
concurrently with blockUntilIdle(): ASTWorker isn't threadsafe
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:97
+ static std::unique_ptr<ParsingCallbacks>
+ capturePreamble(PreambleCallback CB) {
+ class CapturePreamble : public ParsingCallbacks {
----------------
Consider inlining this class until we have a second user.
It's not clear that adapting between lambda and an interface clarifies the test
enough to justify the indirection.
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:97
+ static std::unique_ptr<ParsingCallbacks>
+ capturePreamble(PreambleCallback CB) {
+ class CapturePreamble : public ParsingCallbacks {
----------------
sammccall wrote:
> Consider inlining this class until we have a second user.
> It's not clear that adapting between lambda and an interface clarifies the
> test enough to justify the indirection.
capturePreamble isn't the right name for this function, unlike captureDiags it
doesn't actually send the preamble anywhere.
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:466
updateWithDiags(
- S, File, Inputs, WantDiagnostics::Auto,
+ S, File, Inputs, WantDiagnostics::Yes,
[File, Nonce, &Mut, &TotalUpdates](std::vector<Diag>) {
----------------
As you've pointed out, Yes isn't used in production. So if the behaviour of
Auto has changed we should test that rather than stop using it.
My understanding of the change:
1. we've asserted that every update yields diagnostics
2. this was true because we were calling runWithAST, this forced an AST build
at each snapshot, and we'd publish the diagnostics
3. now we only publish diagnostics once the version has been through the
preamble thread and back
4. these requests get coalesced in the preamble thread if the AST worker is
pushing them faster than the preamble worker is servicing them
5. so we no longer publish diagnostics for every version even if we have a
suitable AST
Is this right?
If so, I'd assert that TotalUpdates is at least FilesCount and at most
FilesCount * UpdatesPerFile, and also that the latest UpdateI for diags (for
any file) is equal to UpdatesPerFile - 1.
And maybe add a brief explanation (we drop diagnostics for some snapshots as
they get coalesced in the preamble worker).
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:999
+ TUScheduler S(CDB, optsForTest(), capturePreamble([&] {
+ if (PreambleBuildCount > 0)
+ Ready.wait();
----------------
This seems like a dubious use of atomic (lack of synchronization between
accesses) - there's actually only one thread here, right?
It would be much clearer IMO to just test the passed-in `Version` directly.
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1020
+ ASSERT_TRUE(bool(AST));
+ EXPECT_THAT(AST->Inputs.Version, PI.Version);
+ RunASTAction.notify();
----------------
nit: spell out "blocking" explicitly
================
Comment at: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp:1020
+ ASSERT_TRUE(bool(AST));
+ EXPECT_THAT(AST->Inputs.Version, PI.Version);
+ RunASTAction.notify();
----------------
sammccall wrote:
> nit: spell out "blocking" explicitly
if you expose preambleVersion() from ParsedAST, you could assert on it
directly. I think this would be much clearer than the stuff with
PreambleBuildCount.
(I don't see any reason not to expose preamble itself, but this probably isn't
a good enough reason to do it)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80293/new/
https://reviews.llvm.org/D80293
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
