This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Avoid <climits> dependency in <thread>
ClosedPublic

Authored by joerg on Mar 29 2021, 9:44 AM.

Details

Summary

The standard guarantees sleep durations of 2^63-1 nanoseconds to work.
Instead of depending on INT64_MAX or ULONGLONG_MAX to exist via the
header pollution, fold the constant directly. That has the additional
positive side effect that it avoids long double arithmetic bugs in GCC.

Diff Detail

Event Timeline

joerg requested review of this revision.Mar 29 2021, 9:44 AM
joerg created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2021, 9:44 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/include/thread
381

Seems reasonable to me, but I do wonder:
(1) Why does the old code even need to drag in long double? What's wrong with regular double? (This is in generic code where 99% of the time __d will just be plain old std::chrono::seconds or std::chrono::milliseconds, which are denominated in long long.)
(2) What do the relevant unit tests look like?

joerg added inline comments.Mar 29 2021, 11:10 AM
libcxx/include/thread
381

I think the idea was to avoid loss of precision or the like if possible. I don't think there is a reasonable way to unit test this?

Well, seems reasonable to me, even if I personally would use double instead of long double.

If my math is right: If the caller passes in "2^53 + 1 nanoseconds" as a long long and we lose precision by casting it to a double with 53 mantissa bits, we end up waiting for only 2^53 nanoseconds (i.e. the precision loss is on the order of one nanosecond every 104 days).

But anyway this seems fine.

Mordante accepted this revision.Mar 30 2021, 12:14 PM
Mordante added a subscriber: Mordante.

LGTM!

This revision is now accepted and ready to land.Mar 30 2021, 12:14 PM

Just curious since the header doesn't include <climits> directly. Why is it required? Do to a compilation error?

Yeah, I hit a compilation error somewhere depending on the include error. I forgot the details, this has been sitting around in my tree for while.

ldionne accepted this revision.Mar 30 2021, 3:01 PM
This revision was automatically updated to reflect the committed changes.

Yeah, I hit a compilation error somewhere depending on the include error. I forgot the details, this has been sitting around in my tree for while.

Thanks for the information.