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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


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


================
Comment at: clangd/TUScheduler.h:92
+              ASTRetentionPolicy RetentionPolicy,
+              DiagsCallback OnDiags = nullptr);
   ~TUScheduler();
----------------
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.


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:32
 
-void ignoreUpdate(Optional<std::vector<Diag>>) {}
 void ignoreError(Error Err) {
   handleAllErrors(std::move(Err), [](const ErrorInfoBase &) {});
----------------
sammccall wrote:
> (up to you if you feel like fixing while here)
> this is just llvm::consumeError
Will fix in a separate commit, like it more when cleanups are the only thing in the commit.


================
Comment at: unittests/clangd/TUSchedulerTests.cpp:38
+/// cancelled.
+void updateWithCallback(TUScheduler &S, PathRef File, ParseInputs Inputs,
+                        WantDiagnostics WD, llvm::unique_function<void()> CB) {
----------------
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).



================
Comment at: unittests/clangd/TUSchedulerTests.cpp:495
       /*UpdateDebounce=*/std::chrono::steady_clock::duration::zero(),
-      ASTRetentionPolicy());
+      ASTRetentionPolicy(), captureDiags);
 
----------------
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54760