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

[Tom via Phabricator via libcxx-commits]

tomcherry marked an inline comment as done.
tomcherry added inline comments.


================
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();
----------------
mclow.lists wrote:
> tomcherry wrote:
> > EricWF wrote:
> > > This assumes the resolution of the `steady_clock`, no?
> > Sort of.  It mostly assumes that steady_clock::now() can be converted into nanoseconds safely, which I think is a reasonable assumption to make.  The old code assumed the resolution of system_clock too, so it's not a regression from there.
> > 
> > I'm happy to hear a better solution, because both this code and the original have never sit well with me.  What we really want is a safe_duration_cast, where we get a ceiling effect if there would be overflow.  Since that is not how std::chrono was created, we're left with two options:
> > 
> > 1) This function ends up with signed integer overflow if someone passes milliseconds::max() to it and steady/system_clock.  
> > 2) Work-arounds like this
> > What we really want is a safe_duration_cast, where we get a ceiling effect if there would be overflow. Since that is not how std::chrono was created, we're left with two options:
> 
> I suggest a third option:  Write `__safe_duration_cast`
> 
> 
I intentionally didn't suggest that since that full solution would be out of the scope of this change, and I find it strange that we'd implement a private version, when we really do need a public safe_duration_cast.

But I did write something similar today: a safe_nanosecond_cast that only handles the cases needed for this code.

I assume that I cannot use __builtin_mul_overflow or similar in libcxx, so I avoided those, though would happily add them back if it were acceptable.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D65339