Page MenuHomePhabricator

[clangd] Provide a way to publish highlightings in non-racy manner
ClosedPublic

Authored by ilya-biryukov on Jul 19 2019, 2:57 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Jul 19 2019, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 2:57 AM

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

clang-tools-extra/clangd/TUScheduler.h
105 ↗(On Diff #210787)

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.

118 ↗(On Diff #210787)

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).
  • Use the same mechanism for diagnostics
  • Change typedef to function<void(function<void()>)>
  • Update a comment
  • s/PublishResults/PublishFn
  • Reformat
ilya-biryukov marked 2 inline comments as done.Jul 19 2019, 5:33 AM
  • Update usage of DiagsMu in a comment
  • Remove a leftover comment from the previous version

Thanks for all the suggestions. This is ready for the next round now.

sammccall accepted this revision.Jul 19 2019, 5:45 AM

Awesome, I think this is in better shape than before highlighting was added.

clang-tools-extra/clangd/TUScheduler.h
102 ↗(On Diff #210807)

uber-nit: newline before this, as currently it's grouped with onPreambleAST rather than onMainAST

This revision is now accepted and ready to land.Jul 19 2019, 5:45 AM
ilya-biryukov marked an inline comment as done.
  • Group PublishFn with onMainAST
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2019, 6:50 AM