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 
extra functionality.
But if you think it would still be more readable the other way, happy to get a 
third opinion.

Comment at: clangd/ClangdServer.h:282
   scheduleReparseAndDiags(PathRef File, VersionedDraft Contents,
+                          WantDiagnostics,
ilya-biryukov wrote:
> 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())
+        Requests.pop_front();
+      assert(!Requests.empty() && "skipped the whole queue");
ilya-biryukov wrote:
> 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 
> one).
> 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 ||
ilya-biryukov wrote:
> 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 
> confusing.
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

Reply via email to