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

[Sam McCall via Phabricator via cfe-commits]

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,
                           Tagged<IntrusiveRefCntPtr<vfs::FileSystem>> TaggedFS);
----------------
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 :-)
Done.
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.
> 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.


================
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.)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43518