[PATCH] D54829: [clangd] Cleanup: make diagnostics callbacks from TUScheduler non-racy

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/ClangdServer.h:132
   /// Remove \p File from list of tracked files, schedule a request to free
-  /// resources associated with it.
+  /// resources associated with it. After this function returns, we guarantee
+  /// that no diagnostics for \p File will be delivered even if there are
----------------
I'm not sure we actually want to *guarantee* this in the API: this implementation serializes removeDocument and the diagnostics callback, but only because it was the most convenient.

I'd suggest just "pending diagnostics for closed files may not be delivered". (and similarly for TUScheduler).

Up to you though.


================
Comment at: clangd/TUScheduler.cpp:256
+  std::mutex DiagsMu;
+  /// When set to false, no diagnostics will be reported. Used to ensure we
+  /// don't report any diagnostics after a shutdown of the worker is requested.
----------------
I think the second sentence is a bit obvious and doesn't get into the subtlety here. Maybe:

```
Used to prevent remove document + leading to out-of-order diagnostics:
The lifetime of the old/new ASTWorkers will overlap, but their handles don't.
When the old handle is destroyed, the old worker will stop reporting diagnostics.
```


================
Comment at: clangd/TUScheduler.cpp:538
+  }
   RequestsCV.notify_all();
 }
----------------
`Done` is covered by `RequestsCV`, and `ReportDiagnostics` is not, it seems a little weird to have the diags critical section in between here.

(I don't have an opinion about the relative order of the sections, but notify_all should immediately follow the `Mutex` section for clarity, I think)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54829