kadircet marked 22 inline comments as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:724
std::move(Req.WantDiags));
+ // Set it after notifying ASTPeer about the preamble to prevent any races.
+ BuiltFirst.notify();
----------------
sammccall wrote:
> hmm, we're notifying the ASTPeer, and then setting builtfirst which is being
> waited on... by astpeer.
> This suggests to me the AST thread should own the notification, not the
> preamble thread.
> And in fact it already has a notification for the first preamble being built!
> why can't we use that one?
umm, it is a little bit more complicated.
The above call only inserts the "update request" onto ASTPeer's queue and
doesn't update the preamble inside ASTPeer. This enables ASTPeer to access
`LatestPreamble` without holding the lock, as it is the only writer. We can
change that, update the `LatestPreamble` while queueing the request.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1133
bool ASTWorker::blockUntilIdle(Deadline Timeout) const {
std::unique_lock<std::mutex> Lock(Mutex);
----------------
sammccall wrote:
> I think this would be clearer as a loop... and maybe more correct.
as discussed offline, loop version isn't more clear keeping the straight
version.
================
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>) {
----------------
sammccall wrote:
> 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).
> Is this right?
yes. in addition to those, previously `Auto` promised a diagnostics release if
it was followed by a read(that's how this test used to work). we no longer
guarantee that for the reasons you've mentioned.
What about keeping `WantDiagnostics::Yes` at all? I believe it should be a
separate patch none the less, but I would be happy with silently converting Yes
to Auto. I would expect this to only break editors not using `Auto` at all and
always forcing diags or not requesting them at all.
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
