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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 3 inline comments as done.
EricWF added a comment.

In https://reviews.llvm.org/D28316#636686, @rmaprath wrote:

> (btw, do those docs updates land on some web URL automagically when committed?).


Assuming the builder isn't broken then yes, the docs should automagically update within ~3 minutes.

> 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?

All of that makes sense. I think you could write those docs better than I could. Would you be willing to update them?



================
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
----------------
rmaprath wrote:
> This is already enforced in the `__config` header (just below where it derives the thread API). I think we can remove this check.
Good catch. Thanks!


================
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
----------------
rmaprath wrote:
> 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` (?).
I think it seems useful enough to make `_LIBCPP_BUILDING_THREAD_LIBRARY_EXTERNAL` work on its own to build a standalone threading library.


================
Comment at: include/__threading_support:38
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
----------------
rmaprath wrote:
> 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.
I think that's already done in the `__config` header if we can't find a threading API.


https://reviews.llvm.org/D28316