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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98015