[libcxx] Simplify rounding of durations in win32 __libcpp_thread_sleep_for

[mstorsjo]

Summary

Also fix a comment typo.

This mirrors what was suggested in review of D97539.

[curdeius]

LGTM! Thanks for coming back to this one.

[curdeius]

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]

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]

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.

[mstorsjo]

Ping for second approval

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

[• Quuxplusone]

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]

libcxx/src/support/win32/thread_win32.cpp

250

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

libcxx/src/support/win32/thread_win32.cpp

 
 void __libcpp_thread_yield()
 {
   SwitchToThread();
 }
 
 void __libcpp_thread_sleep_for(const chrono::nanoseconds& __ns)
 {
-  using namespace chrono;
-  // round-up to the nearest milisecond
-  milliseconds __ms =
-      duration_cast<milliseconds>(__ns + chrono::nanoseconds(999999));
+  // round-up to the nearest millisecond
+  chrono::milliseconds __ms = chrono::ceil<chrono::milliseconds>(__ns);
   // FIXME(compnerd) this should be an alertable sleep (WFSO or SleepEx)
   Sleep(__ms.count());
 }
 
 // Thread Local Storage
 int __libcpp_tls_create(__libcpp_tls_key* __key,
                         void(_LIBCPP_TLS_DESTRUCTOR_CC* __at_exit)(void*))
 {

 {
   return WaitForSingleObjectEx(*(PHANDLE)__sem, INFINITE, false) ==
          WAIT_OBJECT_0;
 }
 
 bool __libcpp_semaphore_wait_timed(__libcpp_semaphore_t* __sem,
                                    chrono::nanoseconds const& __ns)
 {
-  chrono::milliseconds __ms = std::chrono::ceil<chrono::milliseconds>(__ns);
+  chrono::milliseconds __ms = chrono::ceil<chrono::milliseconds>(__ns);
   return WaitForSingleObjectEx(*(PHANDLE)__sem, __ms.count(), false) ==
          WAIT_OBJECT_0;
 }
 
 _LIBCPP_END_NAMESPACE_STD