From: Phil Willoughby Date: Mon, 10 Oct 2016 10:55:14 +0000 (-0700) Subject: Make exception_wrapper::throwException() strict X-Git-Tag: v2016.10.17.00~21 X-Git-Url: http://plrg.eecs.uci.edu/git/?a=commitdiff_plain;h=a62c9841120eb374f8b933d31444a590acee6123;p=folly.git Make exception_wrapper::throwException() strict Summary: The definition of `throwException()` is changed to add the `[[noreturn]]` attribute for consistency with `std::rethrow_exception`. If the `exception_wrapper` contains an exception, that is rethrown; if not, then `std::terminate` is called. An alternative design which threw an exception if the `exception_wrapper` did not contain an exception was rejected because of the ambiguity it introduced as to whether the exception thrown was wrapped or not. Benchmarks before: ``` ============================================================================ folly/test/ExceptionWrapperBenchmark.cpp relative time/iter iters/s ============================================================================ exception_ptr_create_and_test 1.45us 689.01K exception_wrapper_create_and_test 4337.80% 33.46ns 29.89M ---------------------------------------------------------------------------- exception_ptr_create_and_test_concurrent 342.99us 2.92K exception_wrapper_create_and_test_concurrent 101.41% 338.21us 2.96K ---------------------------------------------------------------------------- exception_ptr_create_and_throw 3.05us 327.89K exception_wrapper_create_and_throw 140.22% 2.17us 459.77K exception_wrapper_create_and_cast 8956.80% 34.05ns 29.37M ---------------------------------------------------------------------------- exception_ptr_create_and_throw_concurrent 372.68us 2.68K exception_wrapper_create_and_throw_concurrent 102.54% 363.44us 2.75K exception_wrapper_create_and_cast_concurrent 110.93% 335.97us 2.98K ============================================================================ ``` and after: ``` ============================================================================ folly/test/ExceptionWrapperBenchmark.cpp relative time/iter iters/s ============================================================================ exception_ptr_create_and_test 1.46us 684.09K exception_wrapper_create_and_test 4368.84% 33.46ns 29.89M ---------------------------------------------------------------------------- exception_ptr_create_and_test_concurrent 341.20us 2.93K exception_wrapper_create_and_test_concurrent 99.88% 341.60us 2.93K ---------------------------------------------------------------------------- exception_ptr_create_and_throw 3.08us 324.93K exception_wrapper_create_and_throw 106.93% 2.88us 347.46K exception_wrapper_create_and_cast 9532.05% 32.29ns 30.97M ---------------------------------------------------------------------------- exception_ptr_create_and_throw_concurrent 363.73us 2.75K exception_wrapper_create_and_throw_concurrent 101.04% 360.00us 2.78K exception_wrapper_create_and_cast_concurrent 104.50% 348.07us 2.87K ============================================================================ ``` Reviewed By: yfeldblum Differential Revision: D3923939 fbshipit-source-id: 6fc58ac571e59e4122f1fbd194c678e3a3841057 --- diff --git a/folly/ExceptionWrapper.h b/folly/ExceptionWrapper.h index ca57282c..94f6e828 100644 --- a/folly/ExceptionWrapper.h +++ b/folly/ExceptionWrapper.h @@ -148,12 +148,15 @@ class exception_wrapper { assign_eptr(eptr); } - void throwException() const { + // If the exception_wrapper does not contain an exception, std::terminate() + // is invoked to assure the [[noreturn]] behaviour. + [[noreturn]] void throwException() const { if (throwfn_) { throwfn_(item_.get()); } else if (eptr_) { std::rethrow_exception(eptr_); } + std::terminate(); } explicit operator bool() const { @@ -260,7 +263,9 @@ class exception_wrapper { bool>::type with_exception(F f) const { try { - throwException(); + if (*this) { + throwException(); + } } catch (typename std::decay::type& e) { f(e); return true; @@ -276,7 +281,9 @@ class exception_wrapper { } try { - throwException(); + if (*this) { + throwException(); + } } catch (...) { return std::current_exception(); } diff --git a/folly/test/ExceptionWrapperTest.cpp b/folly/test/ExceptionWrapperTest.cpp index dc3a0ac6..954e92ce 100644 --- a/folly/test/ExceptionWrapperTest.cpp +++ b/folly/test/ExceptionWrapperTest.cpp @@ -170,9 +170,8 @@ TEST(ExceptionWrapper, with_exception_test) { EXPECT_TRUE(bool(ew)); EXPECT_EQ(ew.what(), kIntExceptionClassName + ": int == 23"); EXPECT_EQ(ew.class_name(), kIntExceptionClassName); - ew.with_exception([&](const IntException& ie) { - EXPECT_EQ(ie.getInt(), expected); - }); + EXPECT_TRUE(ew.with_exception( + [&](const IntException& ie) { EXPECT_EQ(ie.getInt(), expected); })); // I can try_and_catch a non-copyable base class. This will use // std::exception_ptr internally. @@ -183,15 +182,21 @@ TEST(ExceptionWrapper, with_exception_test) { EXPECT_TRUE(bool(ew2)); EXPECT_EQ(ew2.what(), kIntExceptionClassName + ": int == 23"); EXPECT_EQ(ew2.class_name(), kIntExceptionClassName); - ew2.with_exception([&](AbstractIntException& ie) { - EXPECT_EQ(ie.getInt(), expected); - EXPECT_TRUE(dynamic_cast(&ie)); - }); + EXPECT_TRUE(ew2.with_exception([&](AbstractIntException& ie) { + EXPECT_EQ(ie.getInt(), expected); + EXPECT_TRUE(dynamic_cast(&ie)); + })); // Test with const this. If this compiles and does not crash due to // infinite loop when it runs, it succeeds. const exception_wrapper& cew = ew; - cew.with_exception([&](const IntException& /* ie */) { SUCCEED(); }); + EXPECT_TRUE( + cew.with_exception([&](const IntException& /* ie */) { SUCCEED(); })); + + // Test with empty ew. + exception_wrapper empty_ew; + EXPECT_FALSE( + empty_ew.with_exception([&](const std::exception& /* ie */) { FAIL(); })); // This won't even compile. You can't use a function which takes a // non-const reference with a const exception_wrapper. @@ -202,6 +207,33 @@ TEST(ExceptionWrapper, with_exception_test) { */ } +TEST(ExceptionWrapper, getExceptionPtr_test) { + int expected = 23; + + // This works, and doesn't slice. + exception_wrapper ew = try_and_catch( + [=]() { throw IntException(expected); }); + std::exception_ptr eptr = ew.getExceptionPtr(); + EXPECT_THROW(std::rethrow_exception(eptr), IntException); + + // I can try_and_catch a non-copyable base class. This will use + // std::exception_ptr internally. + exception_wrapper ew2 = try_and_catch( + [=]() { throw IntException(expected); }); + eptr = ew2.getExceptionPtr(); + EXPECT_THROW(std::rethrow_exception(eptr), IntException); + + // Test with const this. + const exception_wrapper& cew = ew; + eptr = cew.getExceptionPtr(); + EXPECT_THROW(std::rethrow_exception(eptr), IntException); + + // Test with empty ew. + exception_wrapper empty_ew; + eptr = empty_ew.getExceptionPtr(); + EXPECT_DEATH(std::rethrow_exception(eptr), "exception"); +} + TEST(ExceptionWrapper, with_exception_deduction) { auto ew = make_exception_wrapper("hi"); EXPECT_TRUE(ew.with_exception([](std::runtime_error&) {})); @@ -268,6 +300,11 @@ TEST(ExceptionWrapper, exceptionStr) { EXPECT_EQ(kRuntimeErrorClassName + ": argh", exceptionStr(ew)); } +TEST(ExceptionWrapper, throwException_noException) { + exception_wrapper ew; + ASSERT_DEATH(ew.throwException(), "exception"); +} + namespace { class TestException : public std::exception { }; void testEW(const exception_wrapper& ew) {