EricWF added a comment.
I would like to see this patch without the `support/pthread/*.cpp` files. There
are a couple of reasons for this.
1. The symbols in those files are private to the dylib, but they are declared
in the headers and given external visibility.
2. Those symbols frequently use std::chrono types in their interface. This
creates a layering violating between the main STL and the support headers.
3. Moving the implementations of those functions now creates a bunch of noise
in an already noisy patch. Keep the implementations in place for now.
Those changes can be re-added in a follow up patch.
Please combine all everything in `support` into a single file in the top level
directory called `__thread_support` (Or similar, the name really doesn't matter
but it should start with two underscores). Libc++ keeps the number of private
headers is kept to a minimum and only introduces a new one when it needs to
break a cycle or factor out code used in two places.
Overall this is looking good. Have you implemented a support layer for windows
at all? If so I would be curious to see it, I would like to get a better idea
of the end goal.
================
Comment at: include/support/pthread/condition_variable.h:54
@@ +53,3 @@
+_LIBCPP_FUNC_VIS
+void __os_cond_var_timed_wait(__os_cond_var* __cv,
+ __os_mutex* __m,
----------------
This is an internal function. It shouldn't be declared in these headers.
================
Comment at: include/support/pthread/mutex.h:27
@@ +26,3 @@
+typedef pthread_mutex_t __os_mutex;
+_LIBCPP_CONSTEXPR pthread_mutex_t __os_mutex_init = PTHREAD_MUTEX_INITIALIZER;
+
----------------
I think with would work for C++11 onward but unfortunately the mutex internals
need to support C++03. For this reason we have to use a macro to ensure the
initializer is constexpr.
================
Comment at: include/support/pthread/mutex.h:59
@@ +58,3 @@
+_LIBCPP_FUNC_VIS
+void __os_recursive_mutex_init(__os_mutex* __m);
+
----------------
This is an internal function and shouldn't be declared in the headers.
================
Comment at: include/support/pthread/mutex.h:62
@@ +61,3 @@
+inline _LIBCPP_INLINE_VISIBILITY
+int __os_recursive_mutex_lock(__os_mutex* __m)
+{
----------------
Shouldn't these functions take `__os_recursive_mutex*` types?
================
Comment at: include/support/pthread/mutex.h:85
@@ +84,3 @@
+
+_LIBCPP_FUNC_VIS void __os_call_once(volatile unsigned long&, void*,
void(*)(void*));
+
----------------
This is an internal function and shouldn't be declared in the headers.
================
Comment at: include/support/pthread/thread.h:60
@@ +59,3 @@
+{
+ bool res = pthread_equal(t1, t2);
+ return res != 0 ? 0 : (t1 < t2 ? -1 : 1);
----------------
`res` needs to be an int, not a bool.
================
Comment at: include/support/pthread/thread.h:67
@@ +66,3 @@
+basic_ostream<_CharT, _Traits>&
+__os_write_to_ostream(basic_ostream<_CharT, _Traits>& __os, __os_thread_id
__id)
+{
----------------
This function seems superfluous and introduces a dependency on IO streams. Get
rid of it.
================
Comment at: include/support/pthread/thread.h:114
@@ +113,3 @@
+
+_LIBCPP_FUNC_VIS void __os_sleep_for(const chrono::nanoseconds& ns);
+
----------------
This is an internal function and shouldn't be declared in the headers.
================
Comment at: include/thread:238
@@ -236,3 +237,3 @@
bool operator==(__thread_id __x, __thread_id __y) _NOEXCEPT
- {return __x.__id_ == __y.__id_;}
+ {return __libcpp_support::__os_thread_id_compare(__x.__id_, __y.__id_)
== 0;}
friend _LIBCPP_INLINE_VISIBILITY
----------------
This now call's `pthread_equal`, across a library boundary, where it didn't
before. That seems like a pessimization.
================
Comment at: include/thread:244
@@ -242,3 +243,3 @@
bool operator< (__thread_id __x, __thread_id __y) _NOEXCEPT
- {return __x.__id_ < __y.__id_;}
+ {return __libcpp_support::__os_thread_id_compare(__x.__id_,
__y.__id_) < 0;}
friend _LIBCPP_INLINE_VISIBILITY
----------------
Same as above.
================
Comment at: include/thread:260
@@ -258,3 +259,3 @@
operator<<(basic_ostream<_CharT, _Traits>& __os, __thread_id __id)
- {return __os << __id.__id_;}
+ {return __libcpp_support::__os_write_to_ostream(__os, __id.__id_);}
----------------
As mentioned elsewhere just do this operation inline. No need for an extra
function.
================
Comment at: include/thread:393
@@ -390,3 +392,3 @@
std::unique_ptr<_Fp> __p(new _Fp(__f));
- int __ec = pthread_create(&__t_, 0, &__thread_proxy<_Fp>, __p.get());
+ int __ec = __libcpp_support::__os_create_thread(&__t_,
&__thread_proxy<_Gp>, __p.get());
if (__ec == 0)
----------------
Typo `_Gp` should be `_Fp`.
Repository:
rL LLVM
http://reviews.llvm.org/D11781
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
