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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
