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

[Asiri Rathnayake via Phabricator via cfe-commits]

rmaprath added a comment.

In https://reviews.llvm.org/D28316#636700, @EricWF wrote:

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


Sure, please go ahead with the commit. I'll have to do some downstream adjustments first, then I will update the docs.

Thanks for sorting this out!



================
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
----------------
EricWF wrote:
> 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.
Makes sense.


================
Comment at: include/__threading_support:38
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
----------------
EricWF wrote:
> 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.
Indeed, missed that.


https://reviews.llvm.org/D28316