rmaprath closed this revision.
rmaprath added a comment.
@EricWF: Thanks for taking the time to review! :)
Committed as r268734. The bots seem to be happy.
http://reviews.llvm.org/D19412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM after the minor fixes. Thank you for the patch.
Comment at: include/__threading_support:38
@@ +37,3 @@
+{
+pthread_mutexattr_t attr;
+int ec =
rmaprath added a comment.
@mclow.lists, @EricWF: Gentle ping.
http://reviews.llvm.org/D19412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
EricWF added a comment.
In http://reviews.llvm.org/D19412#417729, @rmaprath wrote:
> So, perhaps it is best to leave these pthread mutexes alone, purely for
> performance reasons. We'll have to excuse a couple of `#ifdef
> _LIBCPP_THREAD_API_` conditionals in the library sources to allow
rmaprath added a comment.
In http://reviews.llvm.org/D19412#417682, @EricWF wrote:
> In http://reviews.llvm.org/D19412#416596, @rmaprath wrote:
>
> > In http://reviews.llvm.org/D19412#416183, @EricWF wrote:
> >
> > > OK. I *think* this is my last round of review comments except for one
> > >
EricWF added a comment.
In http://reviews.llvm.org/D19412#416596, @rmaprath wrote:
> In http://reviews.llvm.org/D19412#416183, @EricWF wrote:
>
> > OK. I *think* this is my last round of review comments except for one
> > specific issue. I'm still looking into the changes to the static mutex's
rmaprath updated this revision to Diff 55593.
rmaprath added a comment.
Added missing `__confg` header include.
http://reviews.llvm.org/D19412
Files:
include/__config
include/__mutex_base
include/__threading_support
include/mutex
include/thread
src/algorithm.cpp
rmaprath updated this revision to Diff 55571.
rmaprath added a comment.
Missed one: s/_LIBCPP_COND_INITIALIZER/_LIBCPP_CONDVAR_INITIALIZER
http://reviews.llvm.org/D19412
Files:
include/__config
include/__mutex_base
include/__threading_support
include/mutex
include/thread
rmaprath updated this revision to Diff 55569.
rmaprath added a comment.
Minor tweak: Got rid of the unnecessary template parameters on
`__libcpp_thread_create` and `__libcpp_tl_create`.
http://reviews.llvm.org/D19412
Files:
include/__config
include/__mutex_base
rmaprath updated this revision to Diff 7.
rmaprath added a comment.
In http://reviews.llvm.org/D19412#416183, @EricWF wrote:
> OK. I *think* this is my last round of review comments except for one
> specific issue. I'm still looking into the changes to the static mutex's and
>
EricWF added a comment.
OK. I *think* this is my last round of review comments except for one specific
issue. I'm still looking into the changes to the static mutex's and
condition_variables in `memory.cpp` and `algorithm.cpp`. In these cases I don't
want to go from compile-time initialization
rmaprath updated this revision to Diff 55501.
rmaprath added a comment.
Addressing review comments from @EricWF:
- Agree with all the suggested changes, I've applied them all.
Thanks!
/ Asiri
http://reviews.llvm.org/D19412
Files:
include/__config
include/__mutex_base
EricWF added a comment.
The patch looks good for the most part. Nit picking I would rather have
`__libcpp_thread_foo` instead of `__libcpp_os_support::__os_thread_foo`. IMO
the namespace is undeeded.
@mclow.lists can you weigh in on this?
Comment at: include/__mutex_base:43
bcraig added a comment.
LGTM. You'll still need to wait for one of the other reviewers though
(probably @mclow.lists )
http://reviews.llvm.org/D19412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
rmaprath updated this revision to Diff 55443.
rmaprath added a comment.
Renamed `__os_support` to `__threading_support` as suggested by @mclow.lists
and @bcraig. I've left the namespace name `__libcpp_os_support` untouched, can
change it to `__libcpp_threading_support` if the latter is
bcraig added a comment.
In http://reviews.llvm.org/D19412#414374, @rmaprath wrote:
> > On a bikeshed note: is `<__os_support>` the right name? Or should it be
> > something like `<__thread>` or `<__threading_support>`?
>
>
> I went with `__os_support` in case if we'd want to group further
rmaprath updated this revision to Diff 55425.
rmaprath added a comment.
Addressing review comments by @mclow.lists:
- Switched to `_LIBCPP_ALWAYS_INLINE` from `_LIBCPP_INLINE_VISIBILITY` for all
`__os_xxx` functions.
I've left the remaining points (naming of `__os_support` header and
rmaprath added inline comments.
Comment at: src/algorithm.cpp:51
@@ -50,3 +50,3 @@
#ifndef _LIBCPP_HAS_NO_THREADS
-static pthread_mutex_t __rs_mut = PTHREAD_MUTEX_INITIALIZER;
+static mutex __rs_mut;
#endif
bcraig wrote:
> rmaprath wrote:
> > mclow.lists wrote:
bcraig added inline comments.
Comment at: src/algorithm.cpp:51
@@ -50,3 +50,3 @@
#ifndef _LIBCPP_HAS_NO_THREADS
-static pthread_mutex_t __rs_mut = PTHREAD_MUTEX_INITIALIZER;
+static mutex __rs_mut;
#endif
rmaprath wrote:
> mclow.lists wrote:
> > What happened
rmaprath added a comment.
> On a bikeshed note: is `<__os_support>` the right name? Or should it be
> something like `<__thread>` or `<__threading_support>`?
I went with `__os_support` in case if we'd want to group further external
dependencies (like, for example, if some non-c POSIX calls
mclow.lists added a comment.
In general, I'm happy with this direction. I think we need to check that there
are no ABI changes that happen here (I don't see any, but I'll keep looking),
and I think the stuff in `<__os_support>` should be marked "always inline"
On a bikeshed note: is
bcraig added a comment.
LGTM. You will still need to get approval from one of your original reviewers
though.
http://reviews.llvm.org/D19412
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
rmaprath updated this revision to Diff 55119.
rmaprath added a comment.
Addressing review comments from @bcraig:
In the earlier patch, I tried to keep the `__os_support` header to a minimum by
not exposing the internal pthread dependencies (in library sources) in this
header. This however blew
bcraig added a subscriber: bcraig.
Comment at: src/mutex.cpp:31
@@ +30,3 @@
+#else
+#error "Not implemented for the selected thread API."
+#endif
Can't we just check for _LIBCPP_THREAD_API_PTHREAD once at the top of the file
and #error as necessary there? I
rmaprath updated this revision to Diff 54999.
rmaprath added a comment.
Minor cleanup + Gentle ping.
http://reviews.llvm.org/D19412
Files:
include/__config
include/__mutex_base
include/__os_support
include/mutex
include/thread
src/algorithm.cpp
src/condition_variable.cpp
rmaprath updated this revision to Diff 54658.
rmaprath added a comment.
Added bit more context to the diff.
http://reviews.llvm.org/D19412
Files:
include/__config
include/__mutex_base
include/__os_support
include/mutex
include/thread
src/algorithm.cpp
src/condition_variable.cpp
rmaprath created this revision.
rmaprath added reviewers: EricWF, theraven, mclow.lists, jroelofs.
rmaprath added subscribers: espositofulvio, cfe-commits.
This is mostly D11781 with the final review comments addressed:
- Merged all the headers into a single `__os_support` header
- Moved all
27 matches
Mail list logo
