[PATCH] D54796: [clangd] **Prototype**: C++ API for emitting file status

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/ClangdLSPServer.cpp:787
+  notify("window/showMessage",
+         json::Object {
+           {"uri", URI},
----------------
we should have a protocol model object (in Protocol.h) for the file status updates.

I think it should *likely* it should be distinct from the FileStatus in TUScheduler - optimizing for easy update/comparison and programmatic checks vs optimizing for matching the desired protocol.

Naming is hard, I'd be tempted to take `FileStatus` for the protocol object and and make the TUScheduler one something more obscure like `TUScheduler::TUState`.

Unclear to me which one ClangdServer should expose, my *guess* is we should start by exposing the protocol object, and switch if it doesn't capture enough of the semantics that embedders need.


================
Comment at: clangd/TUScheduler.cpp:338
   auto Task = [=](decltype(OnUpdated) OnUpdated) mutable {
+    OnUpdated(FileStatus{FileStatus::State::Building, "Building"}, {});
     // Will be used to check if we can avoid rebuilding the AST.
----------------
So I'm slightly nervous about just emitting objects directly. The problem is if you have independent properties (e.g. "is using a fallback command" vs "are we computing diagnostics"), then it's a burden on the code here to pass them all every time and a burden on the callback code if you don't.

I think one solution (which *may* be what Ilya's suggesting) is to put a mutable FileStatus object in the ASTWorker, and mutate it in place and then invoke the callback, passing the `const FileStatus&`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54796