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