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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277
       build(std::move(*CurrentReq));
+      bool IsEmpty = false;
+      {
----------------
kadircet wrote:
> 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?
I think the difference is moot - we never create either the AST or preamble worker until there's something to do.

The code around scheduling/sleeping in the AST worker thread is way more complicated, and I'm not confident moving the status broadcast to the top of the loop would be clearer there.

Up to you: if you think both are clearer, move both. If you think the preamble is clearer at the top and AST worker at the bottom, then you can choose between consistency and clarity :-)


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:357
+
+  SynchronizedTUStatus &StatusManager;
 };
----------------
StatusManager -> Status


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:490
 
+  SynchronizedTUStatus StatusManager;
   PreambleThread PW;
----------------
StatusManager -> Status


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:698
+      // buildAST fails.
+      if (!(*AST)) {
+        StatusManager.update(
----------------
(might be clearer to call once and make the logic conditional)


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1053
+  llvm::SmallVector<std::string, 2> Result;
+  if (PA == PreambleAction::Building)
+    Result.push_back("parsing includes");
----------------
nit: write this as a switch  for consistency plus the covered warning?


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:122
 
-  TUAction Action;
-  BuildDetails Details;
+  /// Stores the status of preamble thread.
+  PreambleAction PreambleState = PreambleAction::Idle;
----------------
comment just repeats the declaration, drop it?


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:123
+  /// Stores the status of preamble thread.
+  PreambleAction PreambleState = PreambleAction::Idle;
+  ASTAction ASTState;
----------------
I think these variables are still best described at a high level actions, not states. (Well, the state of the thread is that it is running this action, but that's a little roundabout. TUAction::State probably should have been called Kind instead).

Unfortunate that we need both a type name and a variable name though. `ASTAction ASTActivity; PreambleAction PreambleActivity;`?


================
Comment at: clang-tools-extra/clangd/TUScheduler.h:126
+  /// Stores status of the last build for the translation unit.
+  BuildStatus BS = None;
 };
----------------
If keeping the enum, this variable needs a real name.
I'd suggest `LastBuild` and dropping the comment.


================
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
----------------
kadircet wrote:
> 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.
OK, I understand but don't really agree - the fact that these are exclusive is a coincidence that we don't need to model APIs around. This doesn't seem related to the rest of this patch, either.

I think we're going around in circles on this though, will leave this up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76304