EricWF added a comment.
My main issue with this patch (and https://reviews.llvm.org/D27206) is that
there are now two different CMake options for building two different external
threading libraries. I would much prefer having libc++abi use libc++'s
`__threading_support` header and `cxx_external_threads` library.
================
Comment at: CMakeLists.txt:124
+ This option may only be set to ON when LIBCXXABI_ENABLE_THREADS=ON." OFF)
+option(LIBCXX_HAS_EXTERNAL_THREAD_API
+ "Build libc++ with an externalized threading API.
----------------
It's weird that libc++abi needs to define a libc++ option? Can you elaborate on
the need for this?
================
Comment at: src/CMakeLists.txt:132
+if (LIBCXXABI_HAS_EXTERNAL_THREAD_API)
+ file(GLOB LIBCXXABI_EXTERNAL_THREADING_SUPPORT_SOURCES
../test/support/external_threads.cpp)
+
----------------
Do you really need to glob a single file?
================
Comment at: test/CMakeLists.txt:41
+if (LIBCXXABI_HAS_EXTERNAL_THREAD_API)
+ list(APPEND LIBCXXABI_TEST_DEPS "cxxabi_external_threads")
+endif()
----------------
Target names shouldn't be in quotes.
================
Comment at: test/CMakeLists.txt:45
+if (LIBCXX_HAS_EXTERNAL_THREAD_API)
+ list(APPEND LIBCXXABI_TEST_DEPS "cxx_external_threads")
+endif()
----------------
For the in-tree libc++/libc++abi builds libc++abi gets configured before
libc++, so I don't think libc++ target names are visible in this context.
Are you sure this works?
================
Comment at: test/libcxxabi/test/config.py:43
+ # test_exception_storage_nodynmem.pass.cpp fails under this specific
configuration
+ if self.get_lit_bool('cxxabi_ext_threads', False) and
self.get_lit_bool('libcxxabi_shared', False):
+
self.config.available_features.add('libcxxabi-shared-externally-threaded')
----------------
`libcxxabi_shared` should default to `True` like it does within the libc++
config.
================
Comment at: test/lit.cfg:21
# suffixes: A list of file extensions to treat as test files.
-config.suffixes = ['.cpp', '.s']
+config.suffixes = ['.pass.cpp', '.sh.s', '.sh.cpp']
----------------
I'm not sure I understand the reason for this change.
https://reviews.llvm.org/D27204
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
