ilya-biryukov added inline comments.
================
Comment at: clangd/Threading.h:60
+/// A point in time we may wait for, or None to wait forever.
+/// (We use Optional because buggy implementations of std::chrono overflow...)
+using Deadline = llvm::Optional<std::chrono::steady_clock::time_point>;
----------------
Maybe remove the comment or add more context (i.e. add references) on why the
overflow is buggy?
================
Comment at: clangd/Threading.h:66
+template <typename Func>
+LLVM_NODISCARD bool wait(std::mutex &Mutex, std::condition_variable &CV,
+ Deadline D, Func F) {
----------------
Maybe move this helper to .cpp file?
================
Comment at: clangd/Threading.h:68
+ Deadline D, Func F) {
+ std::unique_lock<std::mutex> Lock(Mutex);
+ if (D)
----------------
Maybe keep the locking part out of this helper? It's often desirable to hold
the lock after `wait()`. This will model how `std::condition_variable::wait` is
defined.
================
Comment at: clangd/Threading.h:83
- void waitForAll();
+ bool waitForAll(Deadline D = llvm::None) const;
void runAsync(UniqueFunction<void()> Action);
----------------
Add `LLVM_NODICARD` here?
For that particular method maybe we could have two overload: with and without
the deadline, i.e.
```
void waitForAll() const;
LLVM_NODISCARD bool waitForAll(Deadline D) const;
```
There are places (like the destructor of this class) where the first overload
is used and consuming the return value is just adding noise, but clients
passing the deadline (e.g. `blockUntilIdle()`) should definitely consume the
return value.
================
Comment at: unittests/clangd/ClangdTests.cpp:784
class NoConcurrentAccessDiagConsumer : public DiagnosticsConsumer {
- public:
- NoConcurrentAccessDiagConsumer(std::promise<void> StartSecondReparse)
- : StartSecondReparse(std::move(StartSecondReparse)) {}
-
- void onDiagnosticsReady(
- PathRef File,
- Tagged<std::vector<DiagWithFixIts>> Diagnostics) override {
+ std::atomic<bool> InCallback = {false};
----------------
Do we need to change this test?
It was specifically designed to keep the second request from overriding the
first one before it was processed.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D43127
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
