sammccall added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:787
+void ClangdLSPServer::onFileUpdated(const FileStatus &FStatus) {
+ notify("window/showMessage", FStatus);
+}
----------------
notifying via `showMessage` looks sensible at first glance as a fallback.
Later we may want to expose a richer API guarded by a client capability, and fallback to `showMessage` if the capability isn't advertised by the client.
Can you verify that `showMessage` is a statusbar-type message and not a dialog in VSCode?
================
Comment at: clangd/ClangdLSPServer.cpp:787
+void ClangdLSPServer::onFileUpdated(const FileStatus &FStatus) {
+ notify("window/showMessage", FStatus);
+}
----------------
sammccall wrote:
> notifying via `showMessage` looks sensible at first glance as a fallback.
> Later we may want to expose a richer API guarded by a client capability, and fallback to `showMessage` if the capability isn't advertised by the client.
>
> Can you verify that `showMessage` is a statusbar-type message and not a dialog in VSCode?
At the moment you've got `FileStatus` seralizing into the wire format of `ShowMessageParams` which seems at least odd.
We should define and pass `ShowMessageParams` here, and the transformation from `FileStatus` -> `ShowMessageParams` should be explicit (a named function or maybe better: inlined here) as it has a different semantic meaning than just `toJSON`.
If the plan is to expose the `FileStatus` object but you're not going to do so in this patch, I think it's OK to leave it in `Protocol.h` - though we should review its structure as if we were going to expose it.
================
Comment at: clangd/ClangdServer.cpp:142
WantDiagnostics WantDiags) {
+ // FIXME: emit PreparingBuild file status.
WorkScheduler.update(File,
----------------
Can you explain how this should be implemented?
It seems like not being able to track PreparingBuild is the main weakness of putting the FileStatus in TUScheduler.
If we can't implement it, fine, but this FIXME suggests it just isn't done yet.
================
Comment at: clangd/ClangdServer.h:39
+// FIXME: find a better name.
class DiagnosticsConsumer {
----------------
hokein wrote:
> jkorous wrote:
> > It would be unfortunate to have this name clashing with `clang::DiagnosticsConsumer` indeed.
> > How about something like `FileEventConsumer`?
> yeah, I plan to rename it later (when the patch gets stable enough). `FileEventConsumer` is a candidate.
The name/problem isn't new; can we do the rename in a separate patch? (before or after). It may have a blast radius in tests etc.
(I'm not sure about `FileEventConsumer` as it's easily confused with something that watches for file changes on disk. My suggestion would be `CompilationWatcher` or `TUWatcher` or something)
================
Comment at: clangd/ClangdServer.h:48
+ /// Called whenever the file status is updated.
+ virtual void onFileUpdated(const FileStatus &FStatus){};
};
----------------
we should have `PathRef File` here too, for consistency.
It's redundant with `FileStatus::uri` but I think that field is actually a layering violation: DiagnosticsConsumer knows about file paths, not URIs.
The more I think about this, the more I think we should move `FileStatus` to the `ClangdServer` or `TUScheduler` layer and exclude URI. If we later want to expose the info via LSP we should define a separate struct in Protocol. WDYT?
================
Comment at: clangd/Protocol.h:937
+enum class FileStatusKind {
+ PreparingBuild = 1, // build system is preparing for building the file, e.g.
----------------
"StatusKind" doesn't carry much meaning.
More subtly, it implies a hierarchy of information in FileStatus. I don't think this reflects reality: if kind = BuildingPreamble and messages = {"has fallback compile command"} then these are peers, not primary/secondary.
I'd suggest calling this `FileAction` instead. In this case you may consider renaming `Ready` -> `Idle` (as "ready" doesn't really tell you what the action is/isn't).
================
Comment at: clangd/Protocol.h:943
+ BuildingFile = 4, // The file is being built.
+ Ready = 5, // The file is ready.
+};
----------------
"ready" also seems a little strong: if a file failed to build, it will be in this state but isn't really "ready" for anything.
================
Comment at: clangd/Protocol.h:947
+/// Clangd extension: a file status indicates the current status of the file in
+/// clangd, sent via the `window/showMessage` notification.
+struct FileStatus {
----------------
(I think this comment doesn't quite make sense: `window/showMessage` is only revelant to clients that receive JSON, and the JSON doesn't follow this structure)
================
Comment at: clangd/Protocol.h:957
+ /// compiler fails to build the AST.
+ std::vector<std::string> messages;
+};
----------------
Some of the most important messages we need to show are warnings (failed compilation, fallback command). These deserve some visual indicator in the client.
You're also sending messages that are unimportant to users ("reusing AST").
I'd suggest either splitting these up into `vector<string> messages`, `vector<string> warnings`, or adding a severity.
================
Comment at: clangd/Protocol.h:957
+ /// compiler fails to build the AST.
+ std::vector<std::string> messages;
+};
----------------
sammccall wrote:
> Some of the most important messages we need to show are warnings (failed compilation, fallback command). These deserve some visual indicator in the client.
> You're also sending messages that are unimportant to users ("reusing AST").
>
> I'd suggest either splitting these up into `vector<string> messages`, `vector<string> warnings`, or adding a severity.
in fact, I'm not sure vector<string> is the right model here at all (for the implementation in TUScheduler) - what happens when you have multiple messages produced at different places?
================
Comment at: clangd/TUScheduler.cpp:261
bool ReportDiagnostics = true; /* GUARDED_BY(DiagMu) */
+ /// Guards a critical section for running the file status callbacks.
+ std::mutex FileStatusMu;
----------------
Why a new mutex rather than reusing ReportDiagnostics?
================
Comment at: clangd/TUScheduler.cpp:351
auto Task = [=]() mutable {
+ auto EmitFileStatus = [&FStatus, this](FileStatusKind State,
+ StringRef Message = "") {
----------------
this seems like it could be a method on ASTWorker instead?
================
Comment at: clangd/TUScheduler.cpp:427
FileName);
+ EmitFileStatus(FileStatusKind::Ready, "reuse prebuilt AST;");
return;
----------------
This looks like the wrong layer to me. Suppose you have a queue like this:
`[update] [go to def] [update] [go to def]`
As implemented, your status is going to be
`building ready building ready`
which seems misleading.
This makes me think:
- the "ready"/"idle" state is a property of the worker thread, not any particular action
- maybe we need another status (running action) and then a string field to tell us which action it was.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54796/new/
https://reviews.llvm.org/D54796