[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:161
+/// update.
+class TUStatusManager {
+public:
----------------
nit: "manager" doesn't really explain what this is, and it's used both as the class name and the main description in the comment.

Maybe TUStatusTracker or something - "Threadsafe holder for TUStatus..."


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:166
+
+  void setPreambleAction(TUAction A) {
+    std::lock_guard<std::mutex> Lock(StatusMu);
----------------
you could consider a slightly more general version:

```
StatusMgr.update([&](TUStatus &S) {
  S.PreambleAction = Idle;
});
```

it's a bit more wordy at the callsite but it makes the correspondence much more direct.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:198
+
+  mutable std::mutex StatusMu;
+  TUStatus Status;
----------------
why mutable with  no const functions?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277
       build(std::move(*CurrentReq));
+      bool IsEmpty = false;
+      {
----------------
Seems clearer to do this immediately before blocking?

at the top:

```
if (!NextReq) {
  Lock.unlock();
  StatusManager.setPreambleAction(Idle);
  Lock.lock();
  ReqCV.wait(NextReq || Done);
}
if (Done)
  break;
```


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:285
+        StatusManager.setPreambleAction({TUAction::Idle, /*Name=*/""});
+      BuiltFirst.notify();
+      ReqCV.notify_all();
----------------
why this change?


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:104
 struct TUStatus {
-  struct BuildDetails {
-    /// Indicates whether clang failed to build the TU.
-    bool BuildFailed = false;
-    /// Indicates whether we reused the prebuilt AST.
-    bool ReuseAST = false;
+  enum BuildStatus {
+    /// Haven't run a built yet
----------------
What's the purpose of this change? It seems to make the build details harder to maintain/extend.

(In particular, say we were going to add a "fallback command was used" bit, where would it go after this change?)


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:117
 
-  TUAction Action;
-  BuildDetails Details;
+  TUAction PreambleAction;
+  TUAction ASTAction;
----------------
why TUAction rather than a PreambleAction enum { Building, Idle }?
It seems Name will never be used, RunningAction/Queued are not possible etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76304