Review comments

Author: ldionne

libcxx/test/std/thread/thread.condition/thread.condition.condvar/wait_for_pred.pass.cpp

@@ -65,12 +65,11 @@ int main(int, char**) {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       std::unique_lock<std::mutex> lock(mutex);
-      lock.unlock();
 
       likely_spurious = false;
+      lock.unlock();
       cv.notify_one();
     });
 
@@ -134,8 +133,7 @@ int main(int, char**) {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       std::unique_lock<std::mutex> lock(mutex);
       lock.unlock();
 
@@ -145,7 +143,8 @@ int main(int, char**) {
       // We would want to assert that the thread has been awoken after this time,
       // however nothing guarantees us that it ever gets spuriously awoken, so
       // we can't really check anything. This is still left here as documentation.
-      assert(awoken || !awoken);
+      bool woke = awoken.load();
+      assert(woke || !woke);
 
       // Whatever happened, actually awaken the condition variable to ensure the test
       // doesn't keep running until the timeout.

libcxx/test/std/thread/thread.condition/thread.condition.condvar/wait_pred.pass.cpp

@@ -48,12 +48,11 @@ int main(int, char**) {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       std::unique_lock<std::mutex> lock(mutex);
-      lock.unlock();
 
       likely_spurious = false;
+      lock.unlock();
       cv.notify_one();
     });
 
@@ -89,8 +88,7 @@ int main(int, char**) {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       std::unique_lock<std::mutex> lock(mutex);
       lock.unlock();
 
@@ -100,7 +98,8 @@ int main(int, char**) {
       // We would want to assert that the thread has been awoken after this time,
       // however nothing guarantees us that it ever gets spuriously awoken, so
       // we can't really check anything. This is still left here as documentation.
-      assert(awoken || !awoken);
+      bool woke = awoken.load();
+      assert(woke || !woke);
 
       // Whatever happened, actually awaken the condition variable to ensure the test finishes.
       cv.notify_one();

libcxx/test/std/thread/thread.condition/thread.condition.condvar/wait_until.pass.cpp

@@ -113,6 +113,5 @@ void test() {
 int main(int, char**) {
   test<TestClock>();
   test<std::chrono::steady_clock>();
-  test<std::chrono::system_clock>();
   return 0;
 }

libcxx/test/std/thread/thread.condition/thread.condition.condvar/wait_until_pred.pass.cpp

@@ -69,12 +69,11 @@ void test() {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       std::unique_lock<std::mutex> lock(mutex);
-      lock.unlock();
 
       likely_spurious = false;
+      lock.unlock();
       cv.notify_one();
     });
 
@@ -134,8 +133,7 @@ void test() {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       std::unique_lock<std::mutex> lock(mutex);
       lock.unlock();
 
@@ -145,7 +143,8 @@ void test() {
       // We would want to assert that the thread has been awoken after this time,
       // however nothing guarantees us that it ever gets spuriously awoken, so
       // we can't really check anything. This is still left here as documentation.
-      assert(awoken || !awoken);
+      bool woke = awoken.load();
+      assert(woke || !woke);
 
       // Whatever happened, actually awaken the condition variable to ensure the test
       // doesn't keep running until the timeout.
@@ -160,6 +159,5 @@ void test() {
 int main(int, char**) {
   test<TestClock>();
   test<std::chrono::steady_clock>();
-  test<std::chrono::system_clock>();
   return 0;
 }

libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_for_pred.pass.cpp

@@ -71,12 +71,11 @@ void test() {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       Lock lock(mutex);
-      lock.unlock();
 
       likely_spurious = false;
+      lock.unlock();
       cv.notify_one();
     });
 
@@ -140,8 +139,7 @@ void test() {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       Lock lock(mutex);
       lock.unlock();
 
@@ -151,7 +149,8 @@ void test() {
       // We would want to assert that the thread has been awoken after this time,
       // however nothing guarantees us that it ever gets spuriously awoken, so
       // we can't really check anything. This is still left here as documentation.
-      assert(awoken || !awoken);
+      bool woke = awoken.load();
+      assert(woke || !woke);
 
       // Whatever happened, actually awaken the condition variable to ensure the test
       // doesn't keep running until the timeout.

libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_pred.pass.cpp

@@ -56,12 +56,11 @@ void test() {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       Lock lock(mutex);
-      lock.unlock();
 
       likely_spurious = false;
+      lock.unlock();
       cv.notify_one();
     });
 
@@ -97,8 +96,7 @@ void test() {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       Lock lock(mutex);
       lock.unlock();
 
@@ -108,7 +106,8 @@ void test() {
       // We would want to assert that the thread has been awoken after this time,
       // however nothing guarantees us that it ever gets spuriously awoken, so
       // we can't really check anything. This is still left here as documentation.
-      assert(awoken || !awoken);
+      bool woke = awoken.load();
+      assert(woke || !woke);
 
       // Whatever happened, actually awaken the condition variable to ensure the test finishes.
       cv.notify_one();

libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_until.pass.cpp

@@ -118,18 +118,14 @@ void test() {
 int main(int, char**) {
   test<std::unique_lock<std::mutex>, TestClock>();
   test<std::unique_lock<std::mutex>, std::chrono::steady_clock>();
-  test<std::unique_lock<std::mutex>, std::chrono::system_clock>();
 
   test<std::unique_lock<std::timed_mutex>, TestClock>();
   test<std::unique_lock<std::timed_mutex>, std::chrono::steady_clock>();
-  test<std::unique_lock<std::timed_mutex>, std::chrono::system_clock>();
 
   test<MyLock<std::mutex>, TestClock>();
   test<MyLock<std::mutex>, std::chrono::steady_clock>();
-  test<MyLock<std::mutex>, std::chrono::system_clock>();
 
   test<MyLock<std::timed_mutex>, TestClock>();
   test<MyLock<std::timed_mutex>, std::chrono::steady_clock>();
-  test<MyLock<std::timed_mutex>, std::chrono::system_clock>();
   return 0;
 }

libcxx/test/std/thread/thread.condition/thread.condition.condvarany/wait_until_pred.pass.cpp

@@ -75,12 +75,11 @@ void test() {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       Lock lock(mutex);
-      lock.unlock();
 
       likely_spurious = false;
+      lock.unlock();
       cv.notify_one();
     });
 
@@ -140,8 +139,7 @@ void test() {
       }
 
       // Acquire the same mutex as t1. This ensures that the condition variable has started
-      // waiting (and hence released that mutex). We don't actually need to hold the lock, we
-      // simply use it as a signal that the condition variable has started waiting.
+      // waiting (and hence released that mutex).
       Lock lock(mutex);
       lock.unlock();
 
@@ -151,7 +149,8 @@ void test() {
       // We would want to assert that the thread has been awoken after this time,
       // however nothing guarantees us that it ever gets spuriously awoken, so
       // we can't really check anything. This is still left here as documentation.
-      assert(awoken || !awoken);
+      bool woke = awoken.load();
+      assert(woke || !woke);
 
       // Whatever happened, actually awaken the condition variable to ensure the test
       // doesn't keep running until the timeout.
@@ -169,22 +168,18 @@ int main(int, char**) {
       support::make_test_thread([] {
         test<std::unique_lock<std::mutex>, TestClock>();
         test<std::unique_lock<std::mutex>, std::chrono::steady_clock>();
-        test<std::unique_lock<std::mutex>, std::chrono::system_clock>();
       }),
       support::make_test_thread([] {
         test<std::unique_lock<std::timed_mutex>, TestClock>();
         test<std::unique_lock<std::timed_mutex>, std::chrono::steady_clock>();
-        test<std::unique_lock<std::timed_mutex>, std::chrono::system_clock>();
       }),
       support::make_test_thread([] {
         test<MyLock<std::mutex>, TestClock>();
         test<MyLock<std::mutex>, std::chrono::steady_clock>();
-        test<MyLock<std::mutex>, std::chrono::system_clock>();
       }),
       support::make_test_thread([] {
         test<MyLock<std::timed_mutex>, TestClock>();
         test<MyLock<std::timed_mutex>, std::chrono::steady_clock>();
-        test<MyLock<std::timed_mutex>, std::chrono::system_clock>();
       })};
 
   for (std::thread& t : tests)