[PATCH] D28229: [libcxx] Fix testing of the externally-threaded library build after r290850

[Asiri Rathnayake via Phabricator via cfe-commits]

rmaprath added a comment.

In https://reviews.llvm.org/D28229#634278, @compnerd wrote:

> I dont think that making Win32 threading an external one makes much sense unless we also do the same for pthreads.  Its just as valid a threading model as pthreads, so why give preferential treatment to that?  (Sure, there are more buildbots using it, but thats not a strong technical justification).


If the Win32 threading is support is going to be upstreamed, yes, it makes sense to give it the same treatment as pthreads (keep it inside `__threading_support`). libcxx has been tied to pthreads for a while, would be good to see something else supported as well. This will also allow us to iron out pthread-isms from the sources into the threading API. I have a couple of patches in that area (which I discovered while implementing the libcxx threading API with a different thread implementation), will put them up for review soon.



================
Comment at: include/__threading_support:30-50
 #if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL) && \
+    !__libcpp_has_include(<__external_threading>)
+// If the <__external_threading> header is absent, build libc++ against a
+// pthread-oriented thread api but leave out its implementation. This setup
+// allows building+testing of an externally-threaded library variant (on any
+// platform that supports pthreads). Here, an 'externally-threaded' library
+// variant is one where the implementation of the libc++ thread api is provided
----------------
compnerd wrote:
> Can you break this down into this which IMO is far more readable:
> 
>     #if defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
>     #if __libcpp_has_include(<external_threading>)
>     #include <__external_threading>
>     #else
>     // Why pthread?
>     #define _LIBCPP_HAS_THREAD_API_EXTERNAL_PTHREAD
>     #endif
>     #endif
> 
>     #if defined(_LIBCPP_HAS_THREAD_API_PTHREAD) ||
>         defined(_LIBCPP_HAS_THREAD_API_EXTERNAL_PTHREAD)
>     #include <pthread.h>
>     #include <sched.h>
>     #endif
The reason I kept it in the current form is, when `__external_threading` is present, everything else in this header needs to be silenced. This is enforced by the current form because everything else in this header lives inside the `#else` side of the `__external_threading` inclusion.

Sure, the header will be a bit more readable if we arrange it the way you suggested, but then it's easy to add something to the header in a way that does not get removed when `__external_threading` is present.


https://reviews.llvm.org/D28229