sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:155
namespace {
+/// Responsible for building and providing access to the preamble of a TU. This
+/// worker doesn't guarantee that each preamble request will be built, in case
----------------
This doc comment doesn't mention "thread" once :-)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:156
+/// Responsible for building and providing access to the preamble of a TU. This
+/// worker doesn't guarantee that each preamble request will be built, in case
+/// of multiple preamble requests, it will only build the last one. Once stop
is
----------------
move documentation of requestBuild() to that function?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:157
+/// worker doesn't guarantee that each preamble request will be built, in case
+/// of multiple preamble requests, it will only build the last one. Once stop
is
+/// called it will build any remaining request and will exit the run loop.
----------------
Move documentation of stop() semantics to stop()?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:159
+/// called it will build any remaining request and will exit the run loop.
+class PreambleWorker {
+public:
----------------
we may want to call this PreambleThread? Current name emphasizes peer
relationship with ASTWorker, but current code has parent/child relationship.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:171
+
+ void requestBuild(CompilerInvocation *CI, ParseInputs PI) {
+ // If compiler invocation was broken, just fail out early.
----------------
or just update()?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:201
+ /// will unblock even after an unsuccessful build.
+ void waitForFirstPreamble() const { BuiltFirstPreamble.wait(); }
+
----------------
I think all the members in this class with "preamble" in the name can drop that
word without any loss, e.g. `PreambleWorker::waitForFirst()`
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:205
+ /// built or latest attempt resulted in a failure.
+ std::shared_ptr<const PreambleData> getLatestBuiltPreamble() const {
+ std::lock_guard<std::mutex> Lock(Mutex);
----------------
latest()?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:218
+ PreambleRequested.wait(Lock, [this] { return PreambleReq || Done; });
+ // No request means we are done.
+ if (!PreambleReq)
----------------
Why do we rebuild the preamble if stop is requested?
================
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) {
----------------
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)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:291
+ /// valid for Req.
+ std::shared_ptr<const PreambleData> buildPreamble(Request Req) {
+ assert(Req.CI && "Got preamble request with null compiler invocation");
----------------
just build(), to avoid confusion clangd::buildPreamble?
Also consider merging with updateLatestBuiltPreamble, both are small and
private and they always go together.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:295
+ std::shared_ptr<const PreambleData> OldPreamble =
+ Inputs.ForceRebuild ? std::shared_ptr<const PreambleData>()
+ : getLatestBuiltPreamble();
----------------
(nullptr should work here?)
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:312
+
+ mutable std::mutex Mutex;
+ bool Done = false;
----------------
Yikes, lots of state :-)
I'd consider merging the two CVs - I find keeping the events separate and
reasoning when you need notify_one vs _all doesn't seem to pay off, may just be
the way I think about queues. Up to you.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:322
+ mutable std::condition_variable PreambleBuilt;
+ std::shared_ptr<const PreambleData> LastBuiltPreamble;
+
----------------
nit: last vs latest, be consistent
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:325
+ Notification BuiltFirstPreamble;
+ const PathRef FileName;
+ ParsingCallbacks &Callbacks;
----------------
please make this owning for simplicity, this isn't a lightweight object anyway
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:600
Inputs, CompilerInvocationDiagConsumer, &CC1Args);
+ auto OldPreamble = PW.getLatestBuiltPreamble();
+ PW.requestBuild(Invocation.get(), Inputs);
----------------
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.
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
