sammccall added inline comments.
================
Comment at: clangd/TUScheduler.cpp:424
if (*AST) {
OnUpdated((*AST)->getDiagnostics());
trace::Span Span("Running main AST callback");
----------------
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.
================
Comment at: clangd/TUScheduler.cpp:426
trace::Span Span("Running main AST callback");
Callbacks.onMainAST(FileName, **AST);
DiagsWereReported = true;
----------------
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.
================
Comment at: clangd/TUScheduler.cpp:515
void ASTWorker::stop() {
+ ShuttingDown = true;
----------------
I think we need to add public docs to TUScheduler::remove and
ClangdServer::removeDocument indicating that diagnostics may not be delivered
after documents are removed (regardless of WantDiagnostics).
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D54829
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
