[PATCH] D21968: [libcxx] Externally threaded libc++ variant - Take 2

[Saleem Abdulrasool via cfe-commits]

compnerd added inline comments.

================
Comment at: CMakeLists.txt:372
@@ +371,3 @@
+    # Relax this restriction from HandleLLVMOptions
+    string(REPLACE "-Wl,-z,defs" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
+  endif()
----------------
There is a similar change in swift to deal with this pollution :-(.

================
Comment at: include/__config:830
@@ -829,1 +829,3 @@
+    !defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && \
+    !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
 # if defined(__FreeBSD__) || \
----------------
I meant on the lines that you added :-).

================
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);
 
----------------
I think we should explicitly spell out `tls`.  The extra character won't hurt.

================
Comment at: lib/CMakeLists.txt:216
@@ +215,3 @@
+  )
+endif()
+
----------------
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.

================
Comment at: test/support/external_threads.cpp:26
@@ +25,3 @@
+    if (__ec)
+      goto fail;
+
----------------
I think this would be nicer as:

  if (int error = pthread_mutexattr_init(&attr))
    return error;

================
Comment at: test/support/external_threads.cpp:52
@@ +51,3 @@
+fail:
+    return __ec;
+}
----------------
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.

================
Comment at: test/support/external_threads.cpp:70
@@ +69,3 @@
+
+int __libcpp_mutex_destroy(__libcpp_mutex_t* __m) {
+    return pthread_mutex_destroy(__m);
----------------
Please choose a single style.

================
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;
----------------
Please choose a single style (and below).

================
Comment at: test/support/external_threads.cpp:131
@@ +130,3 @@
+    // before.
+    if (*__t == 0)
+        return -1;
----------------
Can you compare to `nullptr` instead?

================
Comment at: test/support/external_threads.cpp:142
@@ +141,3 @@
+    // before.
+    if (*__t == 0)
+        return -1;
----------------
Same.


https://reviews.llvm.org/D21968