jroelofs added inline comments. ================ Comment at: include/__config:742 @@ +741,3 @@ +#ifndef _LIBCPP_HAS_NO_THREADS +# if defined(__FreeBSD__) || \ + defined(__NetBSD__) || \ ---------------- jroelofs wrote: > espositofulvio wrote: > > theraven wrote: > > > espositofulvio wrote: > > > > jroelofs wrote: > > > > > espositofulvio wrote: > > > > > > jroelofs wrote: > > > > > > > jroelofs wrote: > > > > > > > > @espositofulvio: @ed meant this: > > > > > > > > > > > > > > > > ``` > > > > > > > > #ifndef _WIN32 > > > > > > > > # include <unistd.h> > > > > > > > > # if _POSIX_THREADS > 0 > > > > > > > > ... > > > > > > > > # endif > > > > > > > > #endif > > > > > > > > ``` > > > > > > > > > > > > > > > > Which //is// the correct way to test for this. > > > > > > > That being said, there have been discussions before about whether > > > > > > > or not we should #include <unistd.h> in <__config>, with the > > > > > > > conclusion being that we shouldn't. > > > > > > > > > > > > > > It would be better if this were a CMake configure-time check that > > > > > > > sets _LIBCPP_THREAD_API, rather than these build-time guards. > > > > > > Tried adding that as configure time checks, but then libcxxabi > > > > > > fails to compile because of the guard in __config to check that > > > > > > _LIBCPP_THREAD_API has beed defined when _LIBCPP_HAS_NO_THREADS is > > > > > > not. > > > > > > > > > > > > As a side note: Is Windows the only OS which hasn't got unistd.h? > > > > > > Tried adding that as configure time checks... > > > > > > > > > > Can you put the patch for that up on gist.github.com, or a > > > > > pastebin?... I'll take a look. > > > > > > > > > > > As a side note: Is Windows the only OS which hasn't got unistd.h? > > > > > > > > > > For the platforms libcxx currently builds on, yes. > > > > > Can you put the patch for that up on gist.github.com, or a > > > > > pastebin?... I'll take a look. > > > > > > > > It's here https://gist.github.com/espositofulvio/eac2fb08acf2e430c516 > > > I'm sorry to have triggered this bikeshed. Given that, of the currently > > > supported platforms, Windows is the only one where we want to use threads > > > but don't want to use PTHREADS, I think it's fine to have this check be > > > !WIN32. The important thing is having the check *in one place* so that > > > the person who wants to add the *second* non-pthread threading > > > implementation doesn't have a load of different places to look: they can > > > just change it in `__config` and then chase all of the compile failures. > > Given that it's something I suggested in a comment on the first revision, I > > was happy to try it out. It was also a quick change. Unfortunately it > > brakes libcxxabi build (at least the way I've done it), so I'm happy to > > make the change if @jroelofs finds another way > I meant something like this: > https://gist.github.com/jroelofs/279cb2639ad910b953d2 > > ... which doesn't quite work yet, but should give you the idea. I don't know > how to get CMake to treat `${CMAKE_BINARY_DIR}/include/c++/v1` as the > includes directory instead of `${CMAKE_CURRENT_SOURCE_DIR}/../include`. Maybe > @ericwf can help with that? > > Basically the idea is that we have cmake create a __config_site header which > __config includes. This new header would then have the definitions for these > kinds of configure-time decisions. I tinkered around with this a bit more, and got it to work. I've updated the gist to reflect that.
To simplify things for @espositofulvio, I'm going to split out the __config_site part of this change into its own review. Repository: rL LLVM http://reviews.llvm.org/D11781 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits