rmaprath added inline comments. ================ Comment at: include/__config:830 @@ -829,1 +829,3 @@ + !defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && \ + !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL) # if defined(__FreeBSD__) || \ ---------------- compnerd wrote: > I meant on the lines that you added :-). Will update to reflect rest of the source file.
================ Comment at: include/__threading_support:261 @@ -200,1 +260,3 @@ +void* __libcpp_tl_get(__libcpp_tl_key __key); +void __libcpp_tl_set(__libcpp_tl_key __key, void* __p); ---------------- compnerd wrote: > I think we should explicitly spell out `tls`. The extra character won't hurt. Will fix. ================ Comment at: lib/CMakeLists.txt:216 @@ +215,3 @@ + ) +endif() + ---------------- compnerd wrote: > Have you considered a slightly alternative approach of basically implementing > pthreads as an "external" threading model as well? You could override it, or > take the default implementation that is provided. It avoids having a second > supporting DSO for threading as we could always statically link against it. So, the particulars of this design was discussed and (generally) agreed upon sometime back. Currently pthreads **is** hidden behind the same threading API as the externally threaded variant (on this patch). To override the default (pthread) implementation, you just have to drop in an `__external_threading` header and specify the `-DLIBCXX_HAS_EXTERNAL_THREAD_API` cmake option. There's no need to provide a second DSO if your `__external_threading` header (and your thread-system sources) are available at `libc++` build time. But we have a **requirement** that the library needs to be build-able in such a way you can provide a second DSO holding the implementation of the API. That is, we need to be able to build and ship a library to which customers can drop-in their own thread implementation (i.e. the thread-support DSO). Note that in one of my earlier patches, I proposed an even more de-coupled external threading system where even the types like `__libcpp_mutex_t` were opaque pointers. @mclow.lists opposed that (perhaps rightfully so, now I think about it) and currently these types need to be known at `libc++` compile time. The implementation of the API functions like `__libcpp_mutex_lock()` can be deferred to link time (this is what is provided in the second DSO). Hope that clarifies everything here? ================ Comment at: test/support/external_threads.cpp:26 @@ +25,3 @@ + if (__ec) + goto fail; + ---------------- compnerd wrote: > I think this would be nicer as: > > if (int error = pthread_mutexattr_init(&attr)) > return error; I think I'm following the conventions used in the `libc++` source files here. Will double check tomorrow. ================ Comment at: test/support/external_threads.cpp:52 @@ +51,3 @@ +fail: + return __ec; +} ---------------- compnerd wrote: > I don't think that the label for failure is adding anything since you really > are handling the destruction of the mutex at all failure sites. Not sure what I did here. Will double check and fix. ================ Comment at: test/support/external_threads.cpp:70 @@ +69,3 @@ + +int __libcpp_mutex_destroy(__libcpp_mutex_t* __m) { + return pthread_mutex_destroy(__m); ---------------- compnerd wrote: > Please choose a single style. Will fix. ================ Comment at: test/support/external_threads.cpp:103 @@ +102,3 @@ + +bool __libcpp_thread_id_equal(__libcpp_thread_id t1, __libcpp_thread_id t2) { + return pthread_equal(t1, t2) != 0; ---------------- compnerd wrote: > Please choose a single style (and below). Will fix. ================ Comment at: test/support/external_threads.cpp:131 @@ +130,3 @@ + // before. + if (*__t == 0) + return -1; ---------------- compnerd wrote: > Can you compare to `nullptr` instead? I can't remember exactly why, but I think there was a problem with using `nullptr` here. I will check and get back. ================ Comment at: test/support/external_threads.cpp:142 @@ +141,3 @@ + // before. + if (*__t == 0) + return -1; ---------------- compnerd wrote: > Same. Need to check. https://reviews.llvm.org/D21968 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits