ilya-biryukov added a comment.
> @ilya-biryukov, I hope the current patch is not too big for you to review,
> happy to chat offline if you want (sam and I had a lot of discussions before
> he is OOO).
Picking it up. Mostly NITs from my side and a few questions to better
understand the design. Also happy to chat offline.
================
Comment at: clangd/ClangdServer.h:40
+// FIXME: find a better name.
class DiagnosticsConsumer {
----------------
ClangdServerCallbacks? xD
================
Comment at: clangd/ClangdServer.h:49
+ /// Called whenever the file status is updated.
+ virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
};
----------------
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
================
Comment at: clangd/TUScheduler.cpp:209
+ /// Updates the TUStatus and emits it.
+ void EmitTUStatus(TUAction FAction,
+ const TUStatus::BuildDetails *Detail = nullptr);
----------------
Naming: `emitTUStatus`?
================
Comment at: clangd/TUScheduler.cpp:242
+ /// Status of the TU.
+ TUStatus Status;
----------------
Maybe add a `GUARDED_BY(DiagMu)` comment?
Also maybe put closer to the mutex itself, e.g. right before ReportDiagnostics.
================
Comment at: clangd/TUScheduler.cpp:584
+ Status.Action = std::move(Action);
+ if (Details) {
+ Status.Details = *Details;
----------------
Code style: remove braces around a single-statement if branch
================
Comment at: clangd/TUScheduler.cpp:637
+ if (Requests.empty()) {
+ EmitTUStatus({TUAction::Idle, /*Name*/ ""});
+ }
----------------
NIT: remove braces?
================
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
----------------
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.
================
Comment at: clangd/TUScheduler.h:64
+ };
+ State S;
+ /// The name of the action currently running, e.g. Update, GoToDef, Hover.
----------------
Maybe set default value to avoid unexpected undefined behavior in case someone
forget to initialize the field?
================
Comment at: clangd/TUScheduler.h:73
+ struct BuildDetails {
+ /// clang fails to build the TU.
+ bool buildFailed = false;
----------------
NIT: maybe clarify: `indicates whether clang failed...` or similar?
================
Comment at: clangd/TUScheduler.h:74
+ /// clang fails to build the TU.
+ bool buildFailed = false;
+ /// indicates whether we reuse the prebuilt AST.
----------------
Naming: should this be `BuildFailed`?
================
Comment at: clangd/TUScheduler.h:75
+ bool buildFailed = false;
+ /// indicates whether we reuse the prebuilt AST.
+ bool reuseAST = false;
----------------
NIT: should this be `reused`?
================
Comment at: clangd/TUScheduler.h:76
+ /// indicates whether we reuse the prebuilt AST.
+ bool reuseAST = false;
+ };
----------------
`ReuseAST`?
================
Comment at: unittests/clangd/TUSchedulerTests.cpp:723
+ // We wait long enough that the document gets removed.
+ std::this_thread::sleep_for(std::chrono::milliseconds(50));
+ }
----------------
It seems we could make this test deterministic if we:
1. create a `Notification`
2. make it ready after calling `removeDocument`.
3. wait for it to become ready at the point where we have `sleep_for` now.
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
