Cleanups for condition_variable's waiting functions (#3974)

Co-authored-by: Casey Carter <[email protected]>
microsoft · Sep 21, 2023 · acb342c · acb342c
1 parent 4047639
commit acb342c
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 52 deletions.
diff --git a/stl/inc/latch b/stl/inc/latch
@@ -31,7 +31,7 @@ _STD_BEGIN
 _EXPORT_STD class latch {
 public:
     _NODISCARD static constexpr ptrdiff_t(max)() noexcept {
-        return (1ULL << (sizeof(ptrdiff_t) * CHAR_BIT - 1)) - 1;
+        return PTRDIFF_MAX;
     }
 
     constexpr explicit latch(const ptrdiff_t _Expected) noexcept /* strengthened */ : _Counter{_Expected} {

diff --git a/stl/inc/mutex b/stl/inc/mutex
@@ -574,25 +574,16 @@ public:
     cv_status wait_for(unique_lock<mutex>& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time) {
         // wait for duration
         if (_Rel_time <= chrono::duration<_Rep, _Period>::zero()) {
+            // we don't unlock-and-relock _Lck for this case because it's not observable
             return cv_status::timeout;
         }
-
-        // TRANSITION, ABI: The standard says that we should use a steady clock,
-        // but unfortunately our ABI relies on the system clock.
-        _timespec64 _Tgt;
-        const bool _Clamped     = _To_timespec64_sys_10_day_clamped(_Tgt, _Rel_time);
-        const cv_status _Result = _Wait_until_sys_time(_Lck, &_Tgt);
-        if (_Clamped) {
-            return cv_status::no_timeout;
-        }
-
-        return _Result;
+        return wait_until(_Lck, _To_absolute_time(_Rel_time));
     }
 
     template <class _Rep, class _Period, class _Predicate>
     bool wait_for(unique_lock<mutex>& _Lck, const chrono::duration<_Rep, _Period>& _Rel_time, _Predicate _Pred) {
         // wait for signal with timeout and check predicate
-        return _Wait_until1(_Lck, _To_absolute_time(_Rel_time), _Pred);
+        return wait_until(_Lck, _To_absolute_time(_Rel_time), _Pass_fn(_Pred));
     }
 
     template <class _Clock, class _Duration>
@@ -601,16 +592,25 @@ public:
 #if _HAS_CXX20
         static_assert(chrono::is_clock_v<_Clock>, "Clock type required");
 #endif // _HAS_CXX20
+#if _CONTAINER_DEBUG_LEVEL > 0
+        _STL_VERIFY(
+            _Lck.owns_lock(), "wait_until's caller must own the lock argument (N4958 [thread.condition.condvar]/17)");
+        _STL_VERIFY(_Mtx_current_owns(_Lck.mutex()->_Mymtx()),
+            "wait_until's calling thread must hold the lock argument's mutex (N4958 [thread.condition.condvar]/17)");
+#endif // _CONTAINER_DEBUG_LEVEL > 0
         for (;;) {
             const auto _Now = _Clock::now();
             if (_Abs_time <= _Now) {
+                // we don't unlock-and-relock _Lck for this case because it's not observable
                 return cv_status::timeout;
             }
 
+            // TRANSITION, ABI: should use a steady clock
             _timespec64 _Tgt;
             (void) _To_timespec64_sys_10_day_clamped(_Tgt, _Abs_time - _Now);
-            const cv_status _Result = _Wait_until_sys_time(_Lck, &_Tgt);
-            if (_Result == cv_status::no_timeout) {
+            // Nothing to do to comply with LWG-2135 because std::mutex lock/unlock are nothrow
+            const _Thrd_result _Res = _Cnd_timedwait(_Mycnd(), _Lck.mutex()->_Mymtx(), &_Tgt);
+            if (_Res == _Thrd_result::_Success) {
                 return cv_status::no_timeout;
             }
         }
@@ -623,7 +623,13 @@ public:
 #if _HAS_CXX20
         static_assert(chrono::is_clock_v<_Clock>, "Clock type required");
 #endif // _HAS_CXX20
-        return _Wait_until1(_Lck, _Abs_time, _Pred);
+        while (!_Pred()) {
+            if (wait_until(_Lck, _Abs_time) == cv_status::timeout) {
+                return _Pred();
+            }
+        }
+
+        return true;
     }
 
     // native_handle_type and native_handle() have intentionally been removed. See GH-3820.
@@ -642,41 +648,6 @@ private:
     _Cnd_t _Mycnd() noexcept { // get pointer to _Cnd_internal_imp_t inside _Cnd_storage
         return reinterpret_cast<_Cnd_t>(&_Cnd_storage);
     }
-
-    cv_status _Wait_until_sys_time(unique_lock<mutex>& _Lck, const _timespec64* _Abs_time) {
-        // wait for signal with timeout
-        if (!_Mtx_current_owns(_Lck.mutex()->_Mymtx())) {
-            _Throw_Cpp_error(_OPERATION_NOT_PERMITTED);
-        }
-
-        // Nothing to do to comply with LWG-2135 because std::mutex lock/unlock are nothrow
-        const _Thrd_result _Res = _Cnd_timedwait(_Mycnd(), _Lck.mutex()->_Mymtx(), _Abs_time);
-
-        if (_Res == _Thrd_result::_Success) {
-            return cv_status::no_timeout;
-        } else {
-            return cv_status::timeout;
-        }
-    }
-
-    template <class _Clock, class _Duration, class _Predicate>
-    bool _Wait_until1(
-        unique_lock<mutex>& _Lck, const chrono::time_point<_Clock, _Duration>& _Abs_time, _Predicate& _Pred) {
-        while (!_Pred()) {
-            const auto _Now = _Clock::now();
-            if (_Abs_time <= _Now) {
-                return false;
-            }
-
-            _timespec64 _Tgt;
-            const bool _Clamped = _To_timespec64_sys_10_day_clamped(_Tgt, _Abs_time - _Now);
-            if (_Wait_until_sys_time(_Lck, &_Tgt) == cv_status::timeout && !_Clamped) {
-                return _Pred();
-            }
-        }
-
-        return true;
-    }
 };
 
 struct _UInt_is_zero {

diff --git a/stl/inc/semaphore b/stl/inc/semaphore
@@ -52,7 +52,7 @@ _NODISCARD unsigned long _Semaphore_remaining_timeout(const chrono::time_point<_
     return static_cast<unsigned long>(_Rel_time.count());
 }
 
-inline constexpr ptrdiff_t _Semaphore_max = (1ULL << (sizeof(ptrdiff_t) * CHAR_BIT - 1)) - 1;
+inline constexpr ptrdiff_t _Semaphore_max = PTRDIFF_MAX;
 
 _EXPORT_STD template <ptrdiff_t _Least_max_value = _Semaphore_max>
 class counting_semaphore {