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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:161
+/// update.
+class TUStatusManager {
+public:
----------------
sammccall wrote:
> 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..."
NAMIIIING !!! :(((


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:198
+
+  mutable std::mutex StatusMu;
+  TUStatus Status;
----------------
sammccall wrote:
> why mutable with  no const functions?
I had a readable version at some point, just leftover from those days :P


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277
       build(std::move(*CurrentReq));
+      bool IsEmpty = false;
+      {
----------------
sammccall wrote:
> 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;
> ```
i agree, but I wanted to keep the current semantics. we only start emitting tustatuses after thread starts executing the first request.

happy to change *both* preamblethread and astworker to emit before blocking though. wdyt?


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:285
+        StatusManager.setPreambleAction({TUAction::Idle, /*Name=*/""});
+      BuiltFirst.notify();
+      ReqCV.notify_all();
----------------
sammccall wrote:
> why this change?
this needs to happen after resetting `CurrentReq` and previously this was part of `build`. It makes more sense to hanlde this resetting as part of the worker thread, rather than as part of the preamble build logic.

but I suppose it doesn't belong into this patch. moving it to the https://reviews.llvm.org/D76125


================
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
----------------
sammccall wrote:
> 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?)
this was to narrow down the interface. Currently builddetails is only being used to report buildstatus, and those cases are mutually exclusive(a build can't be both a reuse and failure). Therefore i wanted to make that more explicit.

if we decide to add more details, I would say we should rather get BuildDetails struct back, with a Status enum and a couple more state to signal any other information.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76304