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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
