[libcxx-commits] [PATCH] D98015: [libcxx] Simplify rounding of durations in win32 __libcpp_thread_sleep_for

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a subscriber: ldionne.
Mordante added inline comments.


================
Comment at: libcxx/src/support/win32/thread_win32.cpp:250
+  // round-up to the nearest millisecond
+  chrono::milliseconds __ms = std::chrono::ceil<chrono::milliseconds>(__ns);
   // FIXME(compnerd) this should be an alertable sleep (WFSO or SleepEx)
----------------
Quuxplusone wrote:
> Mordante wrote:
> > curdeius wrote:
> > > mstorsjo wrote:
> > > > curdeius wrote:
> > > > > 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.
> > > > 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?
> > > 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.
> > 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.
> 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.
That seems like good idea. Maybe discuss with @ldionne?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98015