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