[PATCH] D43518: [clangd] Allow embedders some control over when diagnostics are generated.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/TUScheduler.cpp:298
+      while (shouldSkipHeadLocked())
+        Requests.pop_front();
+      assert(!Requests.empty() && "skipped the whole queue");
----------------
sammccall wrote:
> 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.
> > WDYT?
> 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 requests.
> This is true, but it doesn't seem like a practical concern.
Thanks for clarifying. The first bullet point shouldn't be a big a problem. Yes, the new task can remove multiple items from the back of the queue, but the implementation still looks more natural as it only needs to inspect the **last**  item on the queue on each of the outer loop iterations. (While the current implementation has to do an inner loop through multiple items on the queue in addition to the outer loop).

The second point makes it hard, though. I would probably go with calling `pop_front()` when removing the request and signalling empty queue separately.

> This is true, but it doesn't seem like a practical concern.
It isn't, but I still think it's a nice-to-have.


================
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 ||
----------------
sammccall wrote:
> 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.)
Another alternative is to return an iterator to the first non-dead request from this function use it to remove the dead requests in the clients of this function.
This would get rid of the outer loop. WDYT?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43518