rmaprath added inline comments.

================
Comment at: CMakeLists.txt:139
@@ -138,1 +138,3 @@
 option(LIBCXX_HAS_PTHREAD_API "Ignore auto-detection and force use of pthread 
API" OFF)
+option(LIBCXX_HAS_EXTERNAL_THREAD_API
+  "Build libc++ with an externalized threading API.
----------------
compnerd wrote:
> Can you use `cmake_dependent_option` here instead please?
I have two problems with this: 
* Currently we don't use the `CMakeDepedentOption` module anywhere else in the 
llvm project
* This option silently forces the cmake switch in question to the `FORCE` 
(last) argument  when the dependency in question is not satisfied. Our current 
approach on the other hand is to issue an error to the user indicating the 
incompatible options (see hunk below).

I'm slightly leaning towards keeping the current approach. @EricWF: WDYT?



================
Comment at: include/__config:830
@@ -829,1 +829,3 @@
+    !defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && \
+    !defined(_LIBCPP_HAS_THREAD_API_EXTERNAL)
 # if defined(__FreeBSD__) || \
----------------
compnerd wrote:
> clang-format?
Running `clang-format -style=llvm` on this file shows that it's going to do a 
lot of changes to it. Do we want that? Perhaps it should be a separate patch?

Not sure if this is what you meant.


https://reviews.llvm.org/D21968



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to