ilya-biryukov added a comment.
It just hit me that we should probably just wait for the async request to
finish before returning from completion.
We might be doing this to reduce latency a little, but I don't recall us
measuring that this provides a significant performance win.
Would that work?
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1141
+ ReceivedRequestCV.wait(Lock,
+ [this, Num] { return NumRequestsReceived == Num; });
+ NumRequestsReceived = 0;
----------------
Could we wait with timeout here (a really high one, e.g. 10 seconds) here and
assert we did not hit the timeout?
The `wait` helper from `Threading.h` is a nice API to do this.
This would ensure the test never actually runs into an infinite loop in
practice.
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1152
mutable std::mutex Mut;
+ mutable size_t NumRequestsReceived = 0;
mutable std::vector<FuzzyFindRequest> Requests;
----------------
Maybe just use `Requests.size()`?
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1156
-std::vector<FuzzyFindRequest> captureIndexRequests(llvm::StringRef Code) {
+std::vector<FuzzyFindRequest> captureIndexRequests(llvm::StringRef Code,
+ size_t Num = 1) {
----------------
Could you add a comment that the clients have to consume exactly `Num` requests?
I believe anything else is a misuse:
- if there are more than `Num` requests coming in, we'll get a similar data
race to the one we're trying to fix.
- if there are less than `Num` requests we'll wait for the next request
indefinitely
================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1157
+std::vector<FuzzyFindRequest> captureIndexRequests(llvm::StringRef Code,
+ size_t Num = 1) {
clangd::CodeCompleteOptions Opts;
----------------
Could we also assert no more than `Num` requests were captured?
To avoid unintentional misuse of the default call.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68273/new/
https://reviews.llvm.org/D68273
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
