kadircet added a comment.
we've already discussed most of these so no action needed, but here are some of
them before they become completely irrelevant :P
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:457
+ // Give up on this request, releasing resources if any.
+ void abandon() {
+ std::lock_guard<std::mutex> Lock(Mu);
----------------
in the scenario of a shutdown (or destruction of a preamble thread, because
file is closed), i think we also want to call cancellation of the acquire
release here. as things stand unless we're destroying the throttler itself i
think these requests might stick around.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:483
// Wait until stop is called or there is a request.
- ReqCV.wait(Lock, [this] { return NextReq || Done; });
+ ReqCV.wait(Lock, [&] { return NextReq || Done; });
if (Done)
----------------
any reason for capturing locals here now ?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:500
+
+ ReqCV.wait(Lock, [&] { return TState->ready() || Done; });
+ if (Done) {
----------------
we can move the wait into the `if block` above, i think.
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:500
+
+ ReqCV.wait(Lock, [&] { return TState->ready() || Done; });
+ if (Done) {
----------------
kadircet wrote:
> we can move the wait into the `if block` above, i think.
so this implies preamblerequest we issued an acquire for, and the preamble
request we'll be building below might be different. this is not a concern
initially but i think it would be nice to not get cornered when we want to
notify the throttler about such changes.
i guess `PreambleThread::update` would be the natural place to communicate such
changes, if we're overwriting a preamblerequest and there's a pending acquire
we can either cancel and make preamblethread re-issue acquire, or we can have a
new API on throttler to update the request signals. i think, both of these
require having throttlestate as a member. maybe we should do that directly to
remind ourselves of this in the future?
================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1573
+ case PreambleAction::Queued:
+ Result.push_back("headers are queued");
+ break;
----------------
let's use "includes" here, similar to above.
================
Comment at: clang-tools-extra/clangd/TUScheduler.h:114
+ // If it fails (e.g. races), resources are acquired and must be released.
+ virtual CancelFn acquire(llvm::StringRef Filename, AcquireCallback) = 0;
+
----------------
can we make the first parameter a struct instead? i feel like we might end up
adding more parameters describing the reason for the request here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129100/new/
https://reviews.llvm.org/D129100
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
