[PATCH] D25468: [libcxx] Do not declare the thread api when __external_threading is present

[Asiri Rathnayake via cfe-commits]

rmaprath added a comment.

Added some comments to @EricWF's feedback. Will check back tomorrow (falling asleep...)

/ Asiri



================
Comment at: include/__threading_support:25
 // redundancy is intentional.
 #if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
 #if !defined(__clang__) && (_GNUC_VER < 500)
----------------
EricWF wrote:
> If `_LIBCPP_HAS_THREAD_API_EXTERNAL` is defined can't we just assume that `<__external_threading> is present?
So, `_LIBCPP_HAS_THREAD_API_EXTERNAL` is set from the cmake option for building the externally threaded variant. For the proof-of-concept implementation (that we use for running the test suite), we embed a pthread-based external API within `__threading_support` itself and provide its implementation in `test/support/external_threads.cpp` (which is built into a separate library when running the test suite).

That means, in the default externally-threaded variant (POC), we don't have a `__external_threading` header; the idea is to allow library vendors to drop-in a `__external_threading` header without conflicting with upstream sources, and it will be picked up automatically by `__threading_support` header. Moreover, this way we avoid the cost of shipping an additional header which is not necessary for the most common use case (direct pthread dependency).

This is why we need this contraption to detect if the externally threaded library is built with a `__external_threading` header (i.e. for production) or not (the default - vanilla upstream setup, the POC). 

Hope that makes sense? Perhaps I should explain all this in a comment as well?


================
Comment at: include/__threading_support:35
 
-#if !defined(_LIBCPP_EXTERNAL_THREADING)
+#if !defined(_LIBCPP_HAS_EXTERNAL_THREADING_HEADER)
 #include <pthread.h>
----------------
EricWF wrote:
> Instead of using a new macro couldn't this just be `_LIBCPP_HAS_THREAD_API_EXTERNAL`?
I think my comment above covers this point as well.


https://reviews.llvm.org/D25468