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