From 8563eed76b5054ec4d194e88b32d3b820d14c96e Mon Sep 17 00:00:00 2001 From: John Siegel Date: Thu, 6 Jun 2024 13:58:25 -0400 Subject: [PATCH 1/2] Fixed exception dispatch idiom not being recognized in the middle of a function. --- lib/checkexceptionsafety.cpp | 14 +++++++------- test/testexceptionsafety.cpp | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/lib/checkexceptionsafety.cpp b/lib/checkexceptionsafety.cpp index 12aa3f1dac5..8c45f8e3812 100644 --- a/lib/checkexceptionsafety.cpp +++ b/lib/checkexceptionsafety.cpp @@ -379,14 +379,13 @@ void CheckExceptionSafety::rethrowNoCurrentException() if (!function) continue; - // Rethrow can be used in 'exception dispatcher' idiom which is FP in such case - // https://isocpp.org/wiki/faq/exceptions#throw-without-an-object - // We check the beginning of the function with idiom pattern - if (Token::simpleMatch(function->functionScope->bodyStart->next(), "try { throw ; } catch (")) - continue; - + //// Rethrow can be used in 'exception dispatcher' idiom which is FP in such case + //// https://isocpp.org/wiki/faq/exceptions#throw-without-an-object + //// + //// After the idiom is found, continue to the next function scope in the above for loop. for (const Token *tok = function->functionScope->bodyStart->next(); - tok != function->functionScope->bodyEnd; tok = tok->next()) { + tok != function->functionScope->bodyEnd && + !Token::simpleMatch(tok, "try { throw ; } catch ("); tok = tok->next()) { if (Token::simpleMatch(tok, "catch (")) { tok = tok->linkAt(1); // skip catch argument if (Token::simpleMatch(tok, ") {")) @@ -394,6 +393,7 @@ void CheckExceptionSafety::rethrowNoCurrentException() else break; } + if (Token::simpleMatch(tok, "throw ;")) { rethrowNoCurrentExceptionError(tok); } diff --git a/test/testexceptionsafety.cpp b/test/testexceptionsafety.cpp index 03f7c6a6815..9f40fa6766c 100644 --- a/test/testexceptionsafety.cpp +++ b/test/testexceptionsafety.cpp @@ -55,6 +55,9 @@ class TestExceptionSafety : public TestFixture { TEST_CASE(rethrowNoCurrentException1); TEST_CASE(rethrowNoCurrentException2); TEST_CASE(rethrowNoCurrentException3); + TEST_CASE(exceptionDispatchIdiomInMiddleOfFunction); + TEST_CASE(exceptionDispatchIdiomExtraStatements); + TEST_CASE(exceptionDispatchIdiomSubsequentRethrows); } #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__) @@ -449,6 +452,26 @@ class TestExceptionSafety : public TestFixture { "void func3() { throw 0; }"); ASSERT_EQUALS("", errout_str()); } + + void exceptionDispatchIdiomInMiddleOfFunction() { + check("void on_error() { int foo = 5; try { throw; } catch (...) { ; } }"); + ASSERT_EQUALS("", errout.str()); + + check("void on_error() { int foo = 5; try { throw; } catch (const std::exception& e) { ; } }"); + ASSERT_EQUALS("", errout.str()); + } + + void exceptionDispatchIdiomExtraStatements() { + // extra statements aside from the rethrow should be done before/after the idiom rather than inside it. + check("void on_error() { try { int foo = 5; throw; } catch (...) { ; } }"); + ASSERT_EQUALS("[test.cpp:1]: (error) Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std::terminate(). More: https://isocpp.org/wiki/faq/exceptions#throw-without-an-object\n", errout.str()); + } + + void exceptionDispatchIdiomSubsequentRethrows() { + // if you use the exception dispatch idiom, you have already asserted that you are in a catch block, so subsequent rethrows are valid. + check("void on_error() { int foo = 5; try { throw; } catch (...) { ; } throw; }"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestExceptionSafety) From 2715560e6f8bac3393dd06d0e36655b69cd4d257 Mon Sep 17 00:00:00 2001 From: John Siegel Date: Thu, 6 Jun 2024 15:06:01 -0400 Subject: [PATCH 2/2] Fixed mistaken use of errout.string() instead of errout_string() --- lib/checkexceptionsafety.cpp | 1 + test/testexceptionsafety.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/checkexceptionsafety.cpp b/lib/checkexceptionsafety.cpp index 8c45f8e3812..89f936faea0 100644 --- a/lib/checkexceptionsafety.cpp +++ b/lib/checkexceptionsafety.cpp @@ -374,6 +374,7 @@ void CheckExceptionSafety::rethrowNoCurrentException() { logChecker("CheckExceptionSafety::rethrowNoCurrentException"); const SymbolDatabase* const symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope * scope : symbolDatabase->functionScopes) { const Function* function = scope->function; if (!function) diff --git a/test/testexceptionsafety.cpp b/test/testexceptionsafety.cpp index 9f40fa6766c..522e831d495 100644 --- a/test/testexceptionsafety.cpp +++ b/test/testexceptionsafety.cpp @@ -455,22 +455,22 @@ class TestExceptionSafety : public TestFixture { void exceptionDispatchIdiomInMiddleOfFunction() { check("void on_error() { int foo = 5; try { throw; } catch (...) { ; } }"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("", errout_str()); check("void on_error() { int foo = 5; try { throw; } catch (const std::exception& e) { ; } }"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("", errout_str()); } void exceptionDispatchIdiomExtraStatements() { // extra statements aside from the rethrow should be done before/after the idiom rather than inside it. check("void on_error() { try { int foo = 5; throw; } catch (...) { ; } }"); - ASSERT_EQUALS("[test.cpp:1]: (error) Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std::terminate(). More: https://isocpp.org/wiki/faq/exceptions#throw-without-an-object\n", errout.str()); + ASSERT_EQUALS("[test.cpp:1]: (error) Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std::terminate(). More: https://isocpp.org/wiki/faq/exceptions#throw-without-an-object\n", errout_str()); } void exceptionDispatchIdiomSubsequentRethrows() { // if you use the exception dispatch idiom, you have already asserted that you are in a catch block, so subsequent rethrows are valid. check("void on_error() { int foo = 5; try { throw; } catch (...) { ; } throw; }"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("", errout_str()); } };