Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed exception dispatch idiom not being recognized in the middle of a function #6490

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions lib/checkexceptionsafety.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -374,26 +374,27 @@ 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)
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()) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm you could also put this condition in the loop body.

 if (Token::simpleMatch(tok, "try { throw ; } catch ("))
     break;

I don't have a really strong opinion but feel that it's imho a bit preferable since the loop condition will be easier.

if (Token::simpleMatch(tok, "catch (")) {
tok = tok->linkAt(1); // skip catch argument
if (Token::simpleMatch(tok, ") {"))
tok = tok->linkAt(1); // skip catch scope
else
break;
}

if (Token::simpleMatch(tok, "throw ;")) {
rethrowNoCurrentExceptionError(tok);
}
Expand Down
23 changes: 23 additions & 0 deletions test/testexceptionsafety.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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__)
Expand Down Expand Up @@ -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)
Loading