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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Jul 19 04:39:48 PDT 2019

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).

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list