[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

This revision was automatically updated to reflect the committed changes. Closed by commit rL324990: [clangd] Stop exposing Futures from ClangdServer operations. (authored by sammccall, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D43127

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land. LGTM Comment at: unittests/clangd/ClangdTests.cpp:783 public: -NoConcurrentAccessDiagConsumer(std::promise StartSecondReparse) -: StartSecondRepa

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall added inline comments. Comment at: clangd/TUScheduler.cpp:286 } // unlock Mutex. RequestsCV.notify_one(); } ilya-biryukov wrote: > Change to `notify_all()`? Otherwise we might wake up some thread waiting on > `blockUntilIdle()`, but not the work

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall updated this revision to Diff 133868. sammccall marked 3 inline comments as done. sammccall added a comment. Review comments Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43127 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/TUScheduler.cpp clangd

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

ilya-biryukov added inline comments. Comment at: clangd/TUScheduler.cpp:286 } // unlock Mutex. RequestsCV.notify_one(); } Change to `notify_all()`? Otherwise we might wake up some thread waiting on `blockUntilIdle()`, but not the worker thread.

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall 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; ilya-biryukov wrote

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall updated this revision to Diff 133857. sammccall marked 3 inline comments as done. sammccall added a comment. Change AsyncTaskRunner::wait() to be LLVM_NODISCARD when used with a deadline. Restore NoConcurrentDiagnostics test to its former glory (with comments) Other comment fixes. Repo

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

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; Maybe remove th

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall updated this revision to Diff 133615. sammccall added a comment. rebase Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43127 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/TUScheduler.cpp clangd/TUScheduler.h clangd/Threading.cpp clangd/Thread

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall updated this revision to Diff 133609. sammccall added a comment. Tidy up comment, and revert notify_all to notify_one - it was a red herring. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43127 Files: clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/TUSched

[PATCH] D43127: [clangd] Stop exposing Futures from ClangdServer operations.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added subscribers: cfe-commits, ioeric, jkorous-apple, klimek. LSP has asynchronous semantics, being able to block on an async operation completing is unneccesary and leads to tighter coupling with the threading. I