[PATCH] D68273: [clangd] Fix raciness in code completion tests

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1141
+    ReceivedRequestCV.wait(Lock,
+                           [this, Num] { return NumRequestsReceived == Num; });
+    NumRequestsReceived = 0;
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > 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.
> I don't think it is that important, but sure I suppose a failing test is better than a hanging one :D
Totally agree! Thanks!


================
Comment at: clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1161
   completions(Code, {}, Opts);
-  return Requests.consumeRequests();
+  const auto Reqs = Requests.consumeRequests(Num);
+  EXPECT_EQ(Reqs.size(), Num);
----------------
This code got me thinking whether compiler is allowed to do NRVO if the local variable is const.
Given that the object is not const, we definitely cannot call a move constructor to go from `Reqs` to the return value.
OTOH, if NRVO applies here, this shouldn't matter.

No need to do anything, just thinking out loud.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68273/new/

https://reviews.llvm.org/D68273