[PATCH] D54760: [clangd] Cleanup: make the diags callback global in TUScheduler

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Just the context thing. Rest is some optional simplifications.



================
Comment at: clangd/ClangdServer.cpp:77
+    FileIndex *FIndex,
+    llvm::unique_function<void(PathRef, std::vector<Diag>)> OnDiags) {
+  using DiagsCallback = decltype(OnDiags);
----------------
hmm, the double-indirection for diags seems a bit funny, especially since we have a dedicated method on ClangdServer so the lambda is trivial.

Maybe we should just make the CB class nested or friend in ClangdServer, and pass it a ClangdServer*? Then it can access DynamicIdx and consumeDiagnostics directly. The decoupling here isn't that strong to start with.


================
Comment at: clangd/ClangdServer.h:232
+  /// addDocument. Used to avoid races when sending diagnostics to the clients.
+  static Key<ClangdServer::DocVersion> DocVersionKey;
 
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > I'm not sure using context here buys much: there aren't many layers, they're fairly coupled, and this information would look pretty natural in the interfaces.
> > 
> > What about:
> >  - move the definition of DocVersion to TUScheduler
> >  - make DocVersion a member of ParseInputs
> >  - pass (PathRef, DocVersion, vector<Diag>) to the TUScheduler's diag callback
> I'd keep it separate: `ParseInputs` is defined in `ClangdUnit.cpp` and it serves the purpose of defining everything we need to run the compiler.
> Adding `DocVersion` there would make no sense: it's of no use to the actual code doing preamble builds or parsing.
> 
> I would rather aim to get rid of `DocVersion` (for this particular purpose, there are other ways to fix the raciness that the DocVersions workaround).
> WDYT?
Yeah, separating it from ParseInputs makes sense conceptually, and getting rid of it is indeed better.

I don't think this justifies using Context.

Passing it as a separate parameter seems fine to me. Putting it in ParseInputs with a FIXME is a little less principled but avoids interface churn.


================
Comment at: clangd/TUScheduler.cpp:156
 class ASTWorker {
+private:
   friend class ASTWorkerHandle;
----------------
nit: we generally have the private section at the top without explicitly introducing it (as common in LLVM), or at the bottom. I'm fine with either style but would prefer not to add a third.


================
Comment at: clangd/TUScheduler.h:92
+              ASTRetentionPolicy RetentionPolicy,
+              DiagsCallback OnDiags = nullptr);
   ~TUScheduler();
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ISTM the callback would fit nicely on the ASTCallbacks?
> > It even gets plumbed through to ASTWorker correctly by reference.
> Done. This is definitely less plumbing, but IMO the public interface looks a bit worse now.
> Consuming ASTs and diagnostics are two independent concerns and it's a bit awkward to combine them in the same struct.
> 
> But we don't have too many callers, updating those is easy.
(Separate yes, but I'm not sure they're that much more separate than consuming preambles vs ASTs, really. Outside of the 2/1 split of where the data goes today...)


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:38
+/// cancelled.
+void updateWithCallback(TUScheduler &S, PathRef File, ParseInputs Inputs,
+                        WantDiagnostics WD, llvm::unique_function<void()> CB) {
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ParseInputs is always getInputs(File), you could do that here (similarly for `updateWithDiags`) to avoid repetition.
> It's actually a bit more complicated: `getInputs(File, Contents)`, note the second parameter. 
> Done anyway, definitely looks better (and less prone to mistakes!).
> 
> The calls are now `updateWith(S, File, Contents...)`, there's one place that still relies on the `Inputs` though (checks the VFS does not change in the following read).
> 
Exactly, thanks!
(I forgot to mention Contents in the comment, doh)


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:495
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-      ASTRetentionPolicy());
+      ASTRetentionPolicy(), captureDiags);
 
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > can we just increment a counter in the callback, and have DoUpdate look for a delta?
> Maybe... I haven't looked closely into the test, would prefer to keep this change mostly mechanical and avoid digging into the test logic.
> Happy to take a look into simplifying the test separately, of course.
It seems like a trivial change, and it'd be nice not to depend on the new machinery you're introducing here when we don't have to.
I'm not going to insist, but this is just...

```
atomic<unsigned> DiagsReceived;
TUScheduler S(..., [&](Diags) { ++ DiagsReceived }, ...);
...
DoUpdate = [&] -> bool { // as before
  unsigned PrevDiags = DiagsReceived;
  S.update(...);  blockUntilIdle(); // as before
  return DiagsReceived != PrevDiags;
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54760