[libcxx-commits] [PATCH] D65339: Implement std::condition_variable via pthread_cond_clockwait() where available

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: include/__mutex_base:528
+    nanoseconds d = tp.time_since_epoch();
+    if (d > nanoseconds(0x59682F000000E941))
+        d = nanoseconds(0x59682F000000E941);
----------------
tomcherry wrote:
> ldionne wrote:
> > Please pardon my naiveness, but what is that number?
> It's taken from condition_variable.cpp, added in https://reviews.llvm.org/rGaad745a024a3d0fd573ac9649d4c659e6a713702.  
> 
> I tried to keep all of the safety checks around chrono intact.
Lets give it a name so it makes sense. 


================
Comment at: include/__mutex_base:18
 
+#if defined(_LIBCPP_HAS_COND_CLOCKWAIT)
+#include <time.h>
----------------
Can't we include `time.h` unconditionally?


================
Comment at: include/__mutex_base:311
 
+    template <class _Duration>
+        _LIBCPP_METHOD_TEMPLATE_IMPLICIT_INSTANTIATION_VIS
----------------
This deduction is wrong. Either you deduce the entire time point, like we were originally, or you use the timepoint and duration specified by the clock.

You don't specify the clock and deduce the other parameters

Also, fewer overloads is better. We want to declare a  _single_ public overload. We can do the magic behind the scenes .


================
Comment at: include/__mutex_base:357
        chrono::time_point<chrono::system_clock, chrono::nanoseconds>) _NOEXCEPT;
+#if defined(_LIBCPP_HAS_COND_CLOCKWAIT)
+    void __do_timed_wait(unique_lock<mutex>& __lk,
----------------
This can likely be the same implementation as  above.


================
Comment at: include/__mutex_base:474
+    steady_clock::time_point __c_now = steady_clock::now();
+#if defined(_LIBCPP_HAS_COND_CLOCKWAIT)
+    typedef time_point<steady_clock, duration<long double, nano> > __c_tpf;
----------------
This section seems like it can be generalized to work for both halves of this ifdef.
Having the same code tested in all cases is always better than preprocessor branches.

Forgive me if I'm missing something.


================
Comment at: include/__mutex_base:475
+#if defined(_LIBCPP_HAS_COND_CLOCKWAIT)
+    typedef time_point<steady_clock, duration<long double, nano> > __c_tpf;
+    typedef time_point<steady_clock, nanoseconds> __c_tpi;
----------------
`using` instead of `typedef` in new code if you don't mind.


================
Comment at: include/__mutex_base:476
+    typedef time_point<steady_clock, duration<long double, nano> > __c_tpf;
+    typedef time_point<steady_clock, nanoseconds> __c_tpi;
+    __c_tpf _Max = __c_tpi::max();
----------------
This assumes the resolution of the `steady_clock`, no?


================
Comment at: include/__mutex_base:510
+void
+condition_variable::__do_timed_wait(unique_lock<mutex>& lk,
+     chrono::time_point<chrono::steady_clock, chrono::nanoseconds> tp) _NOEXCEPT
----------------
`__lk`, `__tp`, ` __d`, etc. 

Everything that isn't a part of the public API needs a reserved name.


================
Comment at: include/__mutex_base:523
+    typedef decltype(ts.tv_sec) ts_sec;
+    _LIBCPP_CONSTEXPR ts_sec ts_sec_max = numeric_limits<ts_sec>::max();
+    if (s.count() < ts_sec_max)
----------------
`const` works the same in all dialects.


================
Comment at: include/__mutex_base:527
+        ts.tv_sec = static_cast<ts_sec>(s.count());
+        ts.tv_nsec = static_cast<decltype(ts.tv_nsec)>((d - s).count());
+    }
----------------
What does the cast do except silence bugs?

(Same question above).


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D65339