sammccall added a comment.
I think this is the right design. As mentioned offline, I think we can now move
the onDiagnostics call out from TUScheduler into ClangdServer (and remove
onDiagnostics from the callbacks interface).
This is a better layer because the diagnostics callback model is really about
how LSP treats diagnostics, and TUScheduler shouldn't have to care much (maybe
one day this will even live in ClangdLSPServer).
================
Comment at: clang-tools-extra/clangd/TUScheduler.h:105
+ /// when closing the files.
+ using PublishResults = llvm::function_ref<void()>;
+
----------------
I'd consider having the typedef be the full `using PublishFn =
llvm::function_ref<void(llvm::function_ref<void()>)>`
That way the signature of `onMainAST` is simpler, and realistically people are
going to understand how to call Publish by example.
================
Comment at: clang-tools-extra/clangd/TUScheduler.h:118
+ ///
+ /// This callback is run on a worker thread and if we need to send results of
+ /// processing AST to the users, we might run into a race condition with file
----------------
I think we need to explain the API before apologising for it (explaining why
it's this shape etc). And I think this is weird enough that an example would
help.
Something like:
```
When information about the file (diagnostics, syntax highlighting) is published
to clients,
this should be wrapped in Publish, e.g.
void onMainAST(...) {
Highlights = computeHighlights();
Publish([&] { notifyHighlights(Path, Highlights); });
}
This guarantees that clients will see results in the correct sequence if the
file is concurrently closed
and/or reopened. (The lambda passed to Publish() may never run in this case).
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64985/new/
https://reviews.llvm.org/D64985
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
