[PATCH] D28316: [libc++] Cleanup and document <__threading_support>

[Asiri Rathnayake via Phabricator via cfe-commits]

rmaprath added a comment.

This is much better (few nits inline). And many thanks for the docs update (btw, do those docs updates land on some web URL automagically when committed?).

I think we should point out on the docs that `_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL` is only meant for `libc++` developers / testers. The purpose of this library variant is to:

- Demonstrate that `libc++` can be built with threading support externalised. We use `pthreads` as the underlying implementation in this case because that's the one we can easily test on any platform. I don't think anyone wants to distribute this library variant to users; they would rather compile `libc++` for `pthread` directly instead.
- Write tests that applies to the `LIBCXX_HAS_EXTERNAL_THREAD_API` variant. For example, I hope to write a test to check that the produced `libc++` dylib has no references to `pthread_` symbols within it. Such a test will ensure that the core sources of `libc++` remains pthread-free (all threading should be wired through the threading API).

If a library vendors really wants to distribute a variant of `libc++` with the threading support externalised, they should use a custom `__external_threading` header to replicate the behaviour of `_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL`.

Hope that makes sense?



================
Comment at: CMakeLists.txt:172
    This option may only be set to ON when LIBCXX_ENABLE_THREADS=ON." OFF)
+option(LIBCXX_BUILD_EXTERNAL_THREAD_LIBRARY
+    "Build libc++ with an externalized threading library.
----------------
Can you add another check so that this is not used together with `LIBCXX_HAS_EXTERNAL_THREAD_API` ?


================
Comment at: include/__config:894
 // Thread API
-#if !defined(_LIBCPP_HAS_NO_THREADS) && \
-    !defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && \
-    !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
+#if !defined(_LIBCPP_HAS_NO_THREADS)
 # if defined(__FreeBSD__) || \
----------------
// ignore this comment please (phab won't allow me to delete draft comments)


================
Comment at: include/__threading_support:21
+#if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
+# ifdef _LIBCPP_HAS_NO_THREADS
+#   error _LIBCPP_HAS_NO_THREADS cannot be defined with _LIBCPP_HAS_THREAD_API_EXTERNAL
----------------
This is already enforced in the `__config` header (just below where it derives the thread API). I think we can remove this check.


================
Comment at: include/__threading_support:33
+#if defined(_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL) || \
+    defined(_LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL)
 #define _LIBCPP_THREAD_ABI_VISIBILITY _LIBCPP_FUNC_VIS
----------------
Do we need the second check? I think `_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL` is already set through `__config` header (through `__config_site.in`) when this header gets included `external_threads.cpp` (?).


================
Comment at: include/__threading_support:38
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
----------------
Can we add a second check here like:

```
#if defined(_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL) && \
    !defined(_LIBCPP_HAS_THREAD_API_PTHREAD)
#error "Don't know how to declare the thread API without pthreads".
#endif
```
Or we can enforce this on the cmake file itself (or both). This ties up to my comment about `_LIBCPP_HAS_THREAD_LIBRARY_EXTERNAL` only meant to be for `libc++` developers / testers.


https://reviews.llvm.org/D28316