[PATCH] D54796: [clangd] C++ API for emitting file status
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Nov 30 05:26:50 PST 2018
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
More information about the cfe-commits
mailing list