sammccall added a comment.
Not convinced about the flow control stuff - I think you might be
underestimating the complexity of the cancellation-based approach with the
But if you think it would still be more readable the other way, happy to get a
Comment at: clangd/ClangdServer.h:282
scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
> Maybe add a parameter name here?
> It's mostly a personal preference, I tend to copy-paste parameter lists
> between declaration/definition site if I change them. Missing parameter names
> totally break this workflow for me :-)
My preference is to optimise for reading rather than writing the code, and a
name that carries no extra semantics is just noise. But I don't care that much.
Comment at: clangd/TUScheduler.cpp:298
+ while (shouldSkipHeadLocked())
+ assert(!Requests.empty() && "skipped the whole queue");
> Instead of skipping requests here we could try removing them from back of the
> queue in `startTask` (or rewriting the last request instead of adding a new
> It feels the code could be simpler, as we will only ever have to remove a
> single request from the queue. And it could also keep the queue smaller in
> case of many subsequent `Auto` requests.
Having startTask look ahead to find things to cancel was the thing I found most
confusing/limiting in the previous code, so I'd rather not go back there :-)
That said, I did try this first, trying to limit the scope of this patch, but
it got hard.
The main problems are:
- you're not just looking ahead one task, or even to a fixed one. After [auto
no], no cancels no, auto cancels both, read cancels neither. The states and the
state machine are hard to reason about. (unless you just loop over the whole
queue, which seems just as complex)
- the decision of "should task X run" is distributed over time via mutating
state, rather than happening at one point via reads
- when looking at startTask time, you have to reason about the (possibly)
concurrently running task. In run(), no task is running and nothing can be
enqueued, so there's no concurrency issues.
>And it could also keep the queue smaller in case of many subsequent Auto
This is true, but it doesn't seem like a practical concern.
Comment at: clangd/TUScheduler.cpp:339
+ // Used unless followed by an update that generates diagnostics.
+ for (; Next != Requests.end(); ++Next)
+ if (Next->UpdateType == WantDiagnostics::Yes ||
> Maybe skip updates directly in this function and make it return void?
> Calling a function in a loop that loops through elements itself is a little
Returning bool constrains the contract of this class to choosing to run one
item or not, and the type system forces a specific decision.
Returning void leaves the option of purging one or no or multiple items. The
only void function with a clear contract would be "drop all dead requests at
the start", which is too complex to get right in one go. (happy to pull the
loop out of run into such a function if you think it would help, though.)
rCTE Clang Tools Extra
cfe-commits mailing list