EricWF added a comment.
For the most part this LGTM other than the nits mentioned.
================
Comment at: test/libcxxabi/test/config.py:34
+ 'libunwind_src',
+ os.path.join(self.libcxxabi_src_root, '/../libunwind/src'))
----------------
I think the correct default here is `None`, not `../libunwind/src`, since we
might not be using libunwind at all.
================
Comment at: test/libcxxabi/test/config.py:93
+ 'libunwind_headers',
+ os.path.join(self.libunwind_src, '/../include'))
+ if self.get_lit_bool('llvm_unwinder', False):
----------------
I'm not sure if this is default is optimal. I think None might be less
surprising.
================
Comment at: test/lit.site.cfg.in:9
config.cxx_headers = "@LIBCXXABI_LIBCXX_INCLUDES@"
+config.libunwind_src = "@LIBCXXABI_LIBUNWIND_SOURCES@"
+config.libunwind_headers = "@LIBCXXABI_LIBUNWIND_INCLUDES_INTERNAL@"
----------------
I don't think we need the unwind sources anywhere within the test suite. I
would remove this option all together and simply depend on `libunwind_headers`.
================
Comment at: test/lit.site.cfg.in:10
+config.libunwind_src = "@LIBCXXABI_LIBUNWIND_SOURCES@"
+config.libunwind_headers = "@LIBCXXABI_LIBUNWIND_INCLUDES_INTERNAL@"
config.cxx_library_root = "@LIBCXXABI_LIBCXX_LIBRARY_PATH@"
----------------
I think `libunwind_headers` should be the empty string when
LIBCXXABI_LIBUNWIND_INCLUDE_INTERNAL has not been found, instead of expanding
to "LIBCXXABI_LIBUNWIND_INCLUDE_INTERNAL-NOTFOUND"
================
Comment at: test/test_exception_address_alignment.pass.cpp:30
+// Itanium: Largest supported alignment for the system
+#if _LIBUNWIND_ARM_EHABI
+# define EXPECTED_ALIGNMENT 8
----------------
Shouldn't this be an `#ifdef`?
https://reviews.llvm.org/D31178
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
