kadircet marked 16 inline comments as done.
kadircet added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:265
+ /// Updates the TUStatus and emits it. Only called in the worker thread.
+ void emitTUStatus(TUAction Action,
+ const TUStatus::BuildDetails *Details = nullptr) {
----------------
sammccall wrote:
> as discussed a bit in chat, I think this part needs some more thought.
> Currently the ASTWorker and PreambleWorker emit data for the same file with
> some interleaving that's hard to reason about. The state needs an owner.
>
> (e.g. consider a class that holds a TUStatus and exposes an Event<TUStatus>,
> with threadsafe setPreambleAction and setWorkerAction methods, or so)
sent out https://reviews.llvm.org/D76304
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:600
Inputs, CompilerInvocationDiagConsumer, &CC1Args);
+ auto OldPreamble = PW.getLatestBuiltPreamble();
+ PW.requestBuild(Invocation.get(), Inputs);
----------------
sammccall wrote:
> kadircet wrote:
> > sammccall wrote:
> > > this doesn't seem correct (maybe ok in this patch because of the
> > > blocking, but not in general). You're assuming the last available
> > > preamble is the one that the last AST was built with.
> > >
> > > I suppose you can't check the preamble of the current ParsedAST because
> > > it might not be cached, and you nevertheless want to skip rebuild if the
> > > diagnostics are going to be the same. I can't think of anything better
> > > than continuing to hold the shared_ptr for PreambleForLastBuiltAST or
> > > something like that.
> > right, this is just a "hack" to keep this change NFC.
> >
> > in the follow-up patches i am planning to signal whether latest built
> > preamble is reusable for a given `ParseInputs`, and also signal what the
> > AST should be patched with.
> >
> > diagnostics(ast) will only be built if preamble is re-usable.
> > right, this is just a "hack" to keep this change NFC.
>
> can you add a comment about this? (In particular, why it's correct?)
>
> > in the follow-up patches i am planning to signal whether latest built
> > preamble is reusable for a given ParseInputs
>
> Does "reusable" mean "completely valid" (current semantics), or usable with
> some tweaks (e.g. added headers)? It would be good to define some precise
> terminology around this.
>
> > diagnostics(ast) will only be built if preamble is re-usable.
>
> SG
reusable means completely valid.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:412
RanASTCallback = false;
- emitTUStatus({TUAction::BuildingPreamble, TaskName});
log("ASTWorker building file {0} version {1} with command {2}\n[{3}]\n{4}",
----------------
sammccall wrote:
> why are we moving this?
it has been moved into PreambleThread instead, same as the one below handling
the failure.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76125/new/
https://reviews.llvm.org/D76125
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
