ilya-biryukov added inline comments.
================ Comment at: clangd/ClangdServer.h:49 + /// Called whenever the file status is updated. + virtual void onFileUpdated(PathRef File, const TUStatus &Status){}; }; ---------------- hokein wrote: > 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. LG, I believe the `BuildingPreamble` status in particular might be useful on the C++ API level for us. Not sure `showMessage` or `logMessage` is a good idea, the first one will definitely be annoying if it'll be popping up all the time. Having a new method in the LSP LG. ================ Comment at: clangd/TUScheduler.cpp:267 bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */ + /// Status of the TU. + TUStatus Status; /* GUARDED_BY(DiagMu) */ ---------------- The comment duplicates the code, maybe remove it? ================ Comment at: clangd/TUScheduler.cpp:268 + /// Status of the TU. + TUStatus Status; /* GUARDED_BY(DiagMu) */ }; ---------------- Is `Status` actually ever read from multiple threads? I assume it's only accessed on the worker thread, right? I think we can leave out the locks around it and put beside other fields only accessed by the worker thread, i.e. `DiagsWereReported`, etc. PS Sorry go going back and forth, I didn't think about it in the previous review iteration. ================ Comment at: clangd/TUScheduler.cpp:581 + std::lock_guard<std::mutex> Lock(DiagsMu); + // Stop emitting file statuses when the ASTWorker is shutting down. + if (ReportDiagnostics) { ---------------- NIT: Maybe change `Stop emitting` to `Do not emit`? The current wording seems to suggest we're gonna signal some other code to stop doing that... ================ Comment at: clangd/TUScheduler.cpp:636 + if (Requests.empty()) + emitTUStatus({TUAction::Idle, /*Name*/ ""}); } ---------------- Maybe do it outside the lock? The less dependencies between the locks we have, the better and this one seems unnecessary. ================ Comment at: clangd/TUScheduler.h:57 + enum State { + Unknown, + Queued, // The TU is pending in the thread task queue to be built. ---------------- Maybe remove this value and leave out the default constructor too? That would ensure we never fill have `State` field with undefined value without having this enumerator. And FWIW, putting **any** value as the default seems fine even if we want to keep the default ctor, all the users have to set the State anyway. ================ Comment at: clangd/TUScheduler.h:66 + TUAction() = default; + TUAction(State S, llvm::StringRef Name) : S(S), Name(Name) {}; + State S = Unknown; ---------------- Ah, the annoying part about member initializers :-( (Just grumbling, no need to do anything about it) ================ 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 ---------------- hokein wrote: > 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` > That's the confusing part: clangd might be building the ASTs inside `RunningAction` too, but we only choose to report `BuildingFile` during updates. I would get rid of `BuildingFile` and change it to `RunningAction(Diagnostics)` instead, it feels like that would allow avoiding some confusion in the future. ================ Comment at: unittests/clangd/TUSchedulerTests.cpp:702 + TUState(TUAction::Queued), + AllOf(TUState(TUAction::RunningAction), ActionName("Update")), + TUState(TUAction::BuildingPreamble), TUState(TUAction::BuildingFile), ---------------- Using a single matcher with two params could improve readability, e.g. ``` ElementsAre(TUState(TUAction::Queued, ""), TUState(TUAction::RunningAction, "Update"), ...) ``` Feel free to leave as is if you like the current one better, of course. Just a suggestion. 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