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

[Haojian Wu via Phabricator via cfe-commits]

hokein marked an inline comment as not done.
hokein added inline comments.


================
Comment at: clangd/ClangdServer.h:49
+  /// Called whenever the file status is updated.
+  virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
 };
----------------
ilya-biryukov wrote:
> 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.
`showMessage` seems work well via vim YCM, but it is annoying in vscode (as it pops up diags) -- we need to customize this behavior in our own extensions (showing messages in status bar). We need to revisit different ways. 


================
Comment at: clangd/TUScheduler.cpp:268
+  /// Status of the TU.
+  TUStatus Status; /* GUARDED_BY(DiagMu) */
 };
----------------
ilya-biryukov wrote:
> 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.
Unfortunately not, most statuses are emitted via worker thread, but we emit `Queued` status in the main thread...


================
Comment at: clangd/TUScheduler.cpp:636
+      if (Requests.empty())
+        emitTUStatus({TUAction::Idle, /*Name*/ ""});
     }
----------------
ilya-biryukov wrote:
> Maybe do it outside the lock? The less dependencies between the locks we have, the better and this one seems unnecessary.
Is it safe to read the `Requests` out of the lock here?


================
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
----------------
ilya-biryukov wrote:
> 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.
> 
`BuildingFile` might be more natural (understandable) than `Diagnostics`. Two possible ways here:

- we can also report `BuildingFile` for other AST tasks, does it reduce the confusing part? 
- or if you think `Diagnostic` is an important state of `Update`, how about adding a new state `RunningDiagnostic`? `RunningAction` is a higher level information IMO, clangd is running diagnostic inside `RunningAction(Update).`

WDYT?


================
Comment at: clangd/TUScheduler.h:57
+  enum State {
+    Unknown,
+    Queued,           // The TU is pending in the thread task queue to be built.
----------------
ilya-biryukov wrote:
> 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. 
We need the default constructor... When ASTWorker is constructed, we don't know the any state, I'm nervous about putting **any value** as default value (it will hurt the code readability, future readers might be confused -- why the status is `Queued` when constructing).

Adding an `Unknown` seems ok to me, it is a common practice...


================
Comment at: clangd/TUScheduler.h:66
+  TUAction() = default;
+  TUAction(State S, llvm::StringRef Name) : S(S), Name(Name) {};
+  State S = Unknown;
----------------
ilya-biryukov wrote:
> Ah, the annoying part about member initializers :-(
> (Just grumbling, no need to do anything about it)
Yeap, if we use C++14, we can get rid of it.


================
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));
+      }
----------------
ilya-biryukov wrote:
> 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.
> 
Good suggestion, indeed it found a bug in the previous code...


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54796/new/

https://reviews.llvm.org/D54796