[PATCH] D64985: [clangd] Provide a way to publish highlightings in non-racy manner

[Sam McCall via Phabricator via cfe-commits]

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