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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits