[PATCH] D20677: Make it possible to build a -fno-exceptions libc++abi variant.

[Eric Fiselier via cfe-commits]

EricWF added a comment.

Looking good. I would like to see some tests for the methods we have specificly defined to have behavior in -fno-exceptions mode.


================
Comment at: src/cxa_noexception.cpp:27
@@ +26,3 @@
+
+_LIBCXXABI_FUNC_VIS void
+__cxa_increment_exception_refcount(void *thrown_object) throw() {
----------------
I think either all of the functions in this file should have `_LIBCXXABI_FUNC_VIS` or none of them should. My preference is none since the declarations should have them.

================
Comment at: src/cxa_noexception.cpp:29
@@ +28,3 @@
+__cxa_increment_exception_refcount(void *thrown_object) throw() {
+    if (thrown_object != NULL)
+        std::terminate();
----------------
nit: nullptr

================
Comment at: src/cxa_noexception.cpp:45
@@ +44,3 @@
+void
+__cxa_rethrow_primary_exception(void*) { return; }
+
----------------
I think we should terminate if `void*` is non-null.

================
Comment at: test/backtrace_test.pass.cpp:19
@@ -19,2 +19,2 @@
 }
 
----------------
Does this test have any value in -fno-exceptions mode?

================
Comment at: test/libcxxabi/test/config.py:46
@@ +45,3 @@
+        if self.get_lit_bool('enable_exceptions', True):
+            self.cxx.compile_flags += ['-funwind-tables', '-DLIBCXXABI_ENABLE_EXCEPTIONS']
+        else:
----------------
Nit: I would prefer `LIBCXXABI_HAS_NO_EXCEPTIONS` since the default should be on. The fact that a macro is missing shouldn't disable half the tests.

================
Comment at: test/test_vector1.pass.cpp:8
@@ -7,3 +7,3 @@
 //
 //===----------------------------------------------------------------------===//
 
----------------
Don't you want these tests to run? They aren't part of the exception API AFAIK.

================
Comment at: test/uncaught_exceptions.pass.cpp:10
@@ -9,1 +9,3 @@
 
+// UNSUPPORTED: libcxxabi-no-exceptions
+
----------------
We provided a different implementation so we should test it, not disable the test.


http://reviews.llvm.org/D20677