[PATCH] D54796: [clangd] C++ API for emitting file status

[Ilya Biryukov via Phabricator via cfe-commits]

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