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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to