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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Nice!
The biggest suggestion I have is merging the callback into ASTCallbacks. That's awkward in initialization (`makeUpdateCallbacks` probably needs to be called unconditionally with a maybe-null pointer) but it seems like a natural grouping in all the places it gets plumbed around to... and there are a lot!



================
Comment at: clangd/ClangdServer.h:232
+  /// addDocument. Used to avoid races when sending diagnostics to the clients.
+  static Key<ClangdServer::DocVersion> DocVersionKey;
 
----------------
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


================
Comment at: clangd/TUScheduler.h:92
+              ASTRetentionPolicy RetentionPolicy,
+              DiagsCallback OnDiags = nullptr);
   ~TUScheduler();
----------------
ISTM the callback would fit nicely on the ASTCallbacks?
It even gets plumbed through to ASTWorker correctly by reference.


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:32
 
-void ignoreUpdate(Optional<std::vector<Diag>>) {}
 void ignoreError(Error Err) {
   handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
----------------
(up to you if you feel like fixing while here)
this is just llvm::consumeError


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:36
 
+/// Updates a file and executes a callback when the update is finished or
+/// cancelled.
----------------
Yeah, these are messy. I also don't see something better in general.

There's a place or two below where I think we can avoid them without much verbosity, which I think is probably worthwhile even if lots of tests need them :-/


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:38
+/// cancelled.
+void updateWithCallback(TUScheduler &S, PathRef File, ParseInputs Inputs,
+                        WantDiagnostics WD, llvm::unique_function<void()> CB) {
----------------
ParseInputs is always getInputs(File), you could do that here (similarly for `updateWithDiags`) to avoid repetition.


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:495
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-      ASTRetentionPolicy());
+      ASTRetentionPolicy(), captureDiags);
 
----------------
can we just increment a counter in the callback, and have DoUpdate look for a delta?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54760