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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/TUScheduler.cpp:424
     if (*AST) {
       OnUpdated((*AST)->getDiagnostics());
       trace::Span Span("Running main AST callback");
----------------
sammccall wrote:
> as discussed offline, this doesn't guarantee we're not going to send OnUpdated calls for destroyed FileData objects. Intuitively because ShuttingDown may be set e.g. while building the AST, more subtly because atomic only prevents reordering with respect to other atomic operations.
> 
> I believe we need to hold a lock:
> ```
> {
>   mutex_lock Lock(SendDiagnosticsMu);
>   if (SendDiagnostics)
>     OnUpdated(...);
> }
> ```
> and set SendDiagnostics to false under the same lock.
Done. I've made the change before looking at the comment, so using `ReportDiagnostics` name instead of `SendDiagnostics`. Happy to change if you like the latter more.


================
Comment at: clangd/TUScheduler.cpp:426
       trace::Span Span("Running main AST callback");
       Callbacks.onMainAST(FileName, **AST);
       DiagsWereReported = true;
----------------
sammccall wrote:
> This patch prevents the AST from being indexed after removal - for the same reasons, right?
> 
> But we don't prevent onPreambleAST from being called...
> Not sure if it makes sense to fix this, but deserves at least a comment I think.
Added a comment.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54829