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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to