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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks for the comment, the patch should be ready for review.

There is one thing I'm not certain: should we stop emitting the file status when the file is removed (similar to the behavior of diagnostics)? For example, the file is removed while the AST is building. The current behavior of the patch will emit them.



================
Comment at: clangd/ClangdLSPServer.cpp:787
+  notify("window/showMessage",
+         json::Object {
+           {"uri", URI},
----------------
sammccall wrote:
> 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.
SG. Moved `FileStatus` to `Protocol.h`. 


================
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.
----------------
sammccall wrote:
> 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&`.
ASTWorker seems the right place for FileStatus, given that interesting statuses are in `ASTWorker::update`, I think a local `FileStatus` should be good enough.

ASTWorker is not sufficient to emit all interesting statuses -- at least for `PreparingBuild`, we invoke `getCompileCommand` in `ClangdServer::addDocument`...


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54796