This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Simplify rounding of durations in win32 __libcpp_thread_sleep_for
ClosedPublic

Authored by mstorsjo on Mar 5 2021, 12:56 AM.

Details

Summary

Also fix a comment typo.

This mirrors what was suggested in review of D97539.

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Mar 5 2021, 12:56 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2021, 12:56 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
curdeius accepted this revision as: curdeius.Mar 5 2021, 12:59 AM

LGTM! Thanks for coming back to this one.

curdeius added inline comments.Mar 5 2021, 1:04 AM
libcxx/src/support/win32/thread_win32.cpp
250

Just a remark, not specific to this patch.
It seems that in src/ we sometimes use _VSTD:: and sometimes std::, which is pretty much inconsistent.
IMO, we should use _VSTD:: as much as possible.

mstorsjo added inline comments.Mar 5 2021, 1:08 AM
libcxx/src/support/win32/thread_win32.cpp
250

Hmm, right. Would that also go for the references within _LIBCPP_SEMAPHORE_MAX as discussed in https://reviews.llvm.org/D97539#inline-919370?

I'm not entirely sure about the use of _VSTD::, but my impression is that it would matter most within headers (where users of the library could redirect things or something like that), while the implementation in src/ mostly is fixed anyway?

curdeius added inline comments.Mar 5 2021, 5:57 AM
libcxx/src/support/win32/thread_win32.cpp
250

Yes, indeed, it doesn't change much in src. As I said, we're just inconsistent.
For the macros, as in D97539, I don't know really, we have both.

Ping for second approval

Mordante accepted this revision.Mar 16 2021, 12:41 PM
Mordante added a subscriber: Mordante.

LGTM except a minor issue.

libcxx/src/support/win32/thread_win32.cpp
250

I think we should use std:: in the src directory. For consistency I'd like to remove the std:: here. The function argument also uses chrono instead of std::chrono. Maybe also adjust __libcpp_semaphore_wait_timed so they are similar.

This revision is now accepted and ready to land.Mar 16 2021, 12:41 PM
Quuxplusone added inline comments.
libcxx/src/support/win32/thread_win32.cpp
250

The same question came up (and I punted on it) re string in D98097. Personally I think I'd like to see a patch to add std:: consistently throughout src/ (just like we expect from ordinary user code) — to chrono, string, error_code, etc. etc. However, I'm not 100% sure that it would be a harmless no-op; I would want someone to review it that had an attitude of "I positively understand the ramifications and this is definitely OK," not just "well this seems OK."
And even then, would that patch also change the uses of internal names, like __libcpp_tls_key, to std::__libcpp_tls_key and so on? That seems like a disimprovement, readability-wise. So I dunno if it makes sense to change anything at all.

Mordante added inline comments.
libcxx/src/support/win32/thread_win32.cpp
250

That seems like good idea. Maybe discuss with @ldionne?