<p dir="ltr">@mclow.lists: Ping ?<br>
</p>
<div class="gmail_quote">On 24 Aug 2016 22:46, "Asiri Rathnayake via cfe-commits" <<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">rmaprath added inline comments.<br>
<br>
================<br>
Comment at: include/__config:830<br>
@@ -829,1 +829,3 @@<br>
+    !defined(_LIBCPP_HAS_THREAD_<wbr>API_PTHREAD) && \<br>
+    !defined(_LIBCPP_HAS_THREAD_<wbr>API_EXTERNAL)<br>
 # if defined(__FreeBSD__) || \<br>
----------------<br>
compnerd wrote:<br>
> I meant on the lines that you added :-).<br>
Will update to reflect rest of the source file.<br>
<br>
================<br>
Comment at: include/__threading_support:<wbr>261<br>
@@ -200,1 +260,3 @@<br>
+void* __libcpp_tl_get(__libcpp_tl_<wbr>key __key);<br>
+void __libcpp_tl_set(__libcpp_tl_<wbr>key __key, void* __p);<br>
<br>
----------------<br>
compnerd wrote:<br>
> I think we should explicitly spell out `tls`.  The extra character won't hurt.<br>
Will fix.<br>
<br>
================<br>
Comment at: lib/CMakeLists.txt:216<br>
@@ +215,3 @@<br>
+  )<br>
+endif()<br>
+<br>
----------------<br>
compnerd wrote:<br>
> 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.<br>
So, the particulars of this design was discussed and (generally) agreed upon sometime back.<br>
<br>
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_<wbr>API` cmake option.<br>
<br>
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.<br>
<br>
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).<br>
<br>
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).<br>
<br>
Hope that clarifies everything here?<br>
<br>
<br>
<br>
<br>
<br>
<br>
<br>
================<br>
Comment at: test/support/external_threads.<wbr>cpp:26<br>
@@ +25,3 @@<br>
+    if (__ec)<br>
+      goto fail;<br>
+<br>
----------------<br>
compnerd wrote:<br>
> I think this would be nicer as:<br>
><br>
>   if (int error = pthread_mutexattr_init(&attr))<br>
>     return error;<br>
I think I'm following the conventions used in the `libc++` source files here. Will double check tomorrow.<br>
<br>
================<br>
Comment at: test/support/external_threads.<wbr>cpp:52<br>
@@ +51,3 @@<br>
+fail:<br>
+    return __ec;<br>
+}<br>
----------------<br>
compnerd wrote:<br>
> 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.<br>
Not sure what I did here. Will double check and fix.<br>
<br>
================<br>
Comment at: test/support/external_threads.<wbr>cpp:70<br>
@@ +69,3 @@<br>
+<br>
+int __libcpp_mutex_destroy(__<wbr>libcpp_mutex_t* __m) {<br>
+    return pthread_mutex_destroy(__m);<br>
----------------<br>
compnerd wrote:<br>
> Please choose a single style.<br>
Will fix.<br>
<br>
================<br>
Comment at: test/support/external_threads.<wbr>cpp:103<br>
@@ +102,3 @@<br>
+<br>
+bool __libcpp_thread_id_equal(__<wbr>libcpp_thread_id t1, __libcpp_thread_id t2) {<br>
+    return pthread_equal(t1, t2) != 0;<br>
----------------<br>
compnerd wrote:<br>
> Please choose a single style (and below).<br>
Will fix.<br>
<br>
================<br>
Comment at: test/support/external_threads.<wbr>cpp:131<br>
@@ +130,3 @@<br>
+    // before.<br>
+    if (*__t == 0)<br>
+        return -1;<br>
----------------<br>
compnerd wrote:<br>
> Can you compare to `nullptr` instead?<br>
I can't remember exactly why, but I think there was a problem with using `nullptr` here. I will check and get back.<br>
<br>
================<br>
Comment at: test/support/external_threads.<wbr>cpp:142<br>
@@ +141,3 @@<br>
+    // before.<br>
+    if (*__t == 0)<br>
+        return -1;<br>
----------------<br>
compnerd wrote:<br>
> Same.<br>
Need to check.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D21968" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D21968</a><br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/cfe-commits</a><br>
</blockquote></div>