This is an archive of the discontinued LLVM Phabricator instance.

[libc++] avoid a GCC -Wsigned-compare warning where time_t is unsigned
Needs RevisionPublic

Authored by yamt on Feb 2 2023, 12:11 AM.

Details

Reviewers
Mordante
Group Reviewers
Restricted Project
Summary

Note: For some reasons, GCC -Wall enables the warning for C++
while it doesn't for C.

Diff Detail

Event Timeline

yamt created this revision.Feb 2 2023, 12:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 12:11 AM
yamt requested review of this revision.Feb 2 2023, 12:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2023, 12:11 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for your patch! One suggestion.

libcxx/src/condition_variable.cpp
66

This is consistent with the next line.

yamt added inline comments.Feb 12 2023, 7:44 PM
libcxx/src/condition_variable.cpp
66

my understanding is that s.count() here is long long.
ts_sec is time_t, which can be narrower. (eg. int)

Mordante added inline comments.Feb 18 2023, 5:22 AM
libcxx/src/condition_variable.cpp
66

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?

yamt added inline comments.Feb 27 2023, 12:06 AM
libcxx/src/condition_variable.cpp
66

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

Mordante requested changes to this revision.Sep 11 2023, 10:04 AM

We want to finish the review queue for libc++ due to moving to GitHub PRs. Do you want to continue this patch or should I finish if for you?

libcxx/src/condition_variable.cpp
66

Maybe we should use cmp_less which has solved this issue. We require C++20 for the dylib.

This revision now requires changes to proceed.Sep 11 2023, 10:04 AM
yamt added inline comments.Sep 11 2023, 7:44 PM
libcxx/src/condition_variable.cpp
66

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.)

Mordante added inline comments.Nov 1 2023, 10:40 AM
libcxx/src/condition_variable.cpp
66

C++20 is the version of C++ we use so the version from 2020. This means we can use cmp_less.