espositofulvio added inline comments.
================
Comment at: include/__config:742
@@ +741,3 @@
+#ifndef _LIBCPP_HAS_NO_THREADS
+# if defined(__FreeBSD__) || \
+ defined(__NetBSD__) || \
----------------
jroelofs wrote:
> 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.
I was working on your idea and make it to compile as well, I'm just re-running
the test suite to be 100% sure. Looking at your diff it seems there's a
difference in the path.
${CMAKE_BINARY_DIR}/include/c++/v1/__config_site
the above doesn't work for me, I've used:
${CMAKE_CURRENT_SOURCE_DIR}/include/__config_site
Should I update my 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
