This revision was automatically updated to reflect the committed changes.
Closed by commit rL294573: Threading support: externalize sleep_for() function.
(authored by asiri).
Changed prior to commit:
https://reviews.llvm.org/D29630?vs=87669&id=8#toc
Repository:
rL LLVM
https://reviews.l
joerg accepted this revision.
joerg added a comment.
One small issue left, otherwise LGTM.
Comment at: include/__threading_support:187
+_LIBCPP_THREAD_ABI_VISIBILITY
+void __libcpp_thread_sleep_for(const chrono::nanoseconds& ns);
+
Drop the name here.
https:/
rmaprath added a comment.
In https://reviews.llvm.org/D29630#670876, @EricWF wrote:
> In https://reviews.llvm.org/D29630#670873, @rmaprath wrote:
>
> > Sorry for the delay, I've updated the patch with all the comments addressed.
> >
> > @EricWF: Got one question before I commit: The `sleep_for` f
EricWF added a comment.
In https://reviews.llvm.org/D29630#670873, @rmaprath wrote:
> Sorry for the delay, I've updated the patch with all the comments addressed.
>
> @EricWF: Got one question before I commit: The `sleep_for` function in the
> dylib (`thread.cpp`) is now quite small. Is there a
rmaprath updated this revision to Diff 87669.
rmaprath added a comment.
Sorry for the delay, I've updated the patch with all the comments addressed.
@EricWF: Got one question before I commit: The `sleep_for` function in the
dylib (`thread.cpp`) is now quite small. Is there a particular reason to
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM. @joerg has raised some legitimate concerns about the Windows
implementation. However that shouldn't hold this cleanup up, since the
implementation is preexisting.
Com
rmaprath added inline comments.
Comment at: include/__threading_support:593
+ using namespace chrono;
+ milliseconds ms = duration_cast(ns);
+ if (ms.count() == 0 || ns > duration_cast(ms))
joerg wrote:
> Use (ns + 99) so that the cast rounds up.
So, this
joerg added inline comments.
Comment at: include/__threading_support:367
+ ts.tv_sec = ts_sec_max;
+ ts.tv_nsec = giga::num - 1;
+ }
I don't think giga::num makes things any clear compared to just spelling it out.
Comment at: include/
rmaprath created this revision.
Different platforms implement the wait/sleep behaviour in very different ways.
It makes sense to hoist this functionality into the threading API.
I also feel similarly about `hardware_concurrecy()` implementation. Any
thoughts on that?
https://reviews.llvm.org/
