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