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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/ClangdServer.h:49
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 };
----------------
ilya-biryukov wrote:
> Have we thought about the way we might expose statuses via the LSP?
> The `TUStatus` seems a bit too clangd-specific (i.e. `BuildingPreamble`, `ReuseAST` is not something that makes sense in the protocol). Which might be fine on the `ClangdServer` layer, but it feels like we should generalize before sending it over via the LSP
> The TUStatus seems a bit too clangd-specific (i.e. BuildingPreamble, ReuseAST is not something that makes sense in the protocol). 

Yes, this is by design. `TUStatus` presents the internal states of clangd, and would not be exposed via LSP.

Some ways of exposing status via the LSP
- define a separate structure e.g. `FileStatus` in LSP,  render `TUStatus` to it, and sending it to clients (probably we might want to add a new extension `textDocument/filestatus`)
- reuse some existing LSP methods (`window/showMessage`, `window/logMessage`), render `TUStatus` to one of these methods' params.


================
Comment at: clangd/TUScheduler.h:60
+    BuildingPreamble, // The preamble of the TU is being built.
+    BuildingFile,     // The TU is being built.
+    Idle, // Indicates the worker thread is idle, and ready to run any upcoming
----------------
ilya-biryukov wrote:
> What's the fundamental difference between `BuildingFile` and `RunningAction`?
> We will often rebuild ASTs while running various actions that read the preamble.
> 
> Maybe we could squash those two together? One can view diagnostics as an action on the AST, similar to a direct LSP request like findReferences.
They are two different states, you can think `RunningAction` means the AST worker starts processing a task (for example `Update`, `GoToDefinition`) and `BuildingFile` means building the AST (which is one possible step when processing the task).  

In the current implementation, `BuildingPreamble` and `BuildingFile` are only emitted when AST worker processes the `Update` task (as we are mostly interested in TU states of `ASTWorker::update`); for other AST tasks, we just emit `RunningAction` which should be enough.

Given the following requests in the worker queue:
`[Update]`
`[GoToDef]`
`[Update]`
`[CodeComplete]`

statuses we emitted are

`RunningAction(Update) BuildingPreamble BuildingFile`
`RunningAction(GoToDef)`
`RunningAction(Update)  BuildingPreamble BuildingFile`
`RunningAction(GetCurrentPreamble)`
`Idle`



================
Comment at: clangd/TUScheduler.h:64
+  };
+  State S;
+  /// The name of the action currently running, e.g. Update, GoToDef, Hover.
----------------
ilya-biryukov wrote:
> Maybe set default value to avoid unexpected undefined behavior in case someone forget to initialize the field?
Done.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54796/new/

https://reviews.llvm.org/D54796