[libcxx-commits] [PATCH] D143140: [libc++] avoid a GCC -Wsigned-compare warning where time_t is unsigned

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/src/condition_variable.cpp:66
     _LIBCPP_CONSTEXPR ts_sec ts_sec_max = numeric_limits<ts_sec>::max();
-    if (s.count() < ts_sec_max)
-    {
-        ts.tv_sec = static_cast<ts_sec>(s.count());
-        ts.tv_nsec = static_cast<decltype(ts.tv_nsec)>((d - s).count());
-    }
-    else
-    {
-        ts.tv_sec = ts_sec_max;
-        ts.tv_nsec = giga::num - 1;
+    if ((unsigned long long)s.count() < ts_sec_max) {
+      ts.tv_sec  = static_cast<ts_sec>(s.count());
----------------
yamt wrote:
> Mordante wrote:
> > yamt wrote:
> > > Mordante wrote:
> > > > yamt wrote:
> > > > > Mordante wrote:
> > > > > > This is consistent with the next line.
> > > > > my understanding is that s.count() here is long long.
> > > > > ts_sec is time_t, which can be narrower. (eg. int)
> > > > > 
> > > > Good point, `ts_sec`, might indeed be a smaller type. (There is no guarantee count is long long, but it most likely is.)
> > > > 
> > > > I still wonder whether this change fixes anything, in glibc `time_t` is signed
> > > > https://www.gnu.org/software/libc/manual/html_node/Time-Types.html
> > > > 
> > > > Maybe it's safer to cast the result to the common type of these two types, WDYT?
> > > you are right this is not necessary for glibc.
> > > 
> > > however, for other platforms, time_t can be uint32_t, uint64_t, etc.
> > > cf. https://github.com/apache/nuttx/pull/8201
> > Maybe we should use `cmp_less` which has solved this issue. We require C++20 for the dylib.
> > Maybe we should use `cmp_less` which has solved this issue.
> 
> maybe. i wasn't aware of the function. thank you.
> 
> > We require C++20 for the dylib.
> 
> can you explain a bit? (i'm not a C++ expert.)
> 
> 
C++20 is the version of C++ we use so the version from 2020. This means we can use `cmp_less`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143140