From d11b5163b757e0821fe2291e80b2ba8fc3a196fc Mon Sep 17 00:00:00 2001 From: Zachary Blair Date: Fri, 31 Dec 2010 03:01:38 -0800 Subject: [PATCH] Fixed #2382 (Catching exceptions by value instead of reference) --- lib/checkother.cpp | 36 +++++++++++++++++ lib/checkother.h | 7 ++++ test/testother.cpp | 96 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 309ef2f9f..0e99191bb 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -302,6 +302,34 @@ void CheckOther::checkIncorrectLogicOperator() } } +//--------------------------------------------------------------------------- +// try {} catch (std::exception err) {} <- Should be "std::exception& err" +//--------------------------------------------------------------------------- +void CheckOther::checkCatchExceptionByValue() +{ + if (!_settings->_checkCodingStyle) + return; + + const char catchPattern[] = "} catch ("; + const Token *tok = Token::findmatch(_tokenizer->tokens(), catchPattern); + const Token *endTok = tok ? tok->tokAt(2)->link() : NULL; + + while (tok && endTok) + { + // Find a pass-by-value declaration in the catch(), excluding basic types + // e.g. catch (std::exception err) + const Token *tokType = Token::findmatch(tok, "%type% %var% )", endTok); + if (tokType && + !Token::Match(tokType, "bool|char|double|enum|float|int|long|short|size_t|wchar_t")) + { + catchExceptionByValueError(tokType); + } + + tok = Token::findmatch(endTok->next(), catchPattern); + endTok = tok ? tok->tokAt(2)->link() : NULL; + } +} + //--------------------------------------------------------------------------- // strtol(str, 0, radix) <- radix must be 0 or 2-36 //--------------------------------------------------------------------------- @@ -2768,3 +2796,11 @@ void CheckOther::misusedScopeObjectError(const Token *tok, const std::string& va reportError(tok, Severity::error, "unusedScopedObject", "instance of \"" + varname + "\" object destroyed immediately"); } + +void CheckOther::catchExceptionByValueError(const Token *tok) +{ + reportError(tok, Severity::style, + "catchExceptionByStyle", "Exception should be caught by reference.\n" + "The exception is caught as a value. It could be caught " + "as a (const) reference which is usually recommended in C++."); +} diff --git a/lib/checkother.h b/lib/checkother.h index 1117fee42..4af6d528b 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -82,6 +82,7 @@ public: checkOther.checkSelfAssignment(); checkOther.checkIncorrectLogicOperator(); checkOther.checkMisusedScopedObject(); + checkOther.checkCatchExceptionByValue(); } /** @brief Are there C-style pointer casts in a c++ file? */ @@ -162,6 +163,9 @@ public: /** @brief %Check for objects that are destroyed immediately */ void checkMisusedScopedObject(); + /** @brief %Check for exceptions that are caught by value instead of by reference */ + void checkCatchExceptionByValue(); + // Error messages.. void cstyleCastError(const Token *tok); void dangerousUsageStrtolError(const Token *tok); @@ -183,6 +187,7 @@ public: void assignmentInAssertError(const Token *tok, const std::string &varname); void incorrectLogicOperatorError(const Token *tok); void misusedScopeObjectError(const Token *tok, const std::string &varname); + void catchExceptionByValueError(const Token *tok); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) { @@ -218,6 +223,7 @@ public: c.allocatedButUnusedVariableError(0, "varname"); c.unreadVariableError(0, "varname"); c.unassignedVariableError(0, "varname"); + c.catchExceptionByValueError(0); } std::string name() const @@ -254,6 +260,7 @@ public: "* look for calculations inside sizeof()\n" "* assignment of a variable to itself\n" "* mutual exclusion over || always evaluating to true\n" + "* exception caught by value instead of by reference\n" // optimisations "* optimisation: detect post increment/decrement\n"; diff --git a/test/testother.cpp b/test/testother.cpp index 18854d0c6..6a7d9c106 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -93,6 +93,7 @@ private: TEST_CASE(assignmentInAssert); TEST_CASE(incorrectLogicOperator); + TEST_CASE(catchExceptionByValue); } void check(const char code[], const char *filename = NULL) @@ -125,6 +126,7 @@ private: checkOther.invalidScanf(); checkOther.checkMisusedScopedObject(); checkOther.checkIncorrectLogicOperator(); + checkOther.checkCatchExceptionByValue(); } @@ -1518,6 +1520,100 @@ private: ); ASSERT_EQUALS("", errout.str()); } + + void catchExceptionByValue() + { + check("void f() {\n" + " try\n" + " {\n" + " foo();\n" + " }\n" + " catch( ::std::exception err)\n" + " {\n" + " throw err;\n" + " }\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:6]: (style) Exception should be caught by reference.\n", errout.str()); + + check("void f() {\n" + " try\n" + " {\n" + " foo();\n" + " }\n" + " catch(const exception err)\n" + " {\n" + " throw err;\n" + " }\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:6]: (style) Exception should be caught by reference.\n", errout.str()); + + check("void f() {\n" + " try\n" + " {\n" + " foo();\n" + " }\n" + " catch( ::std::exception& err)\n" + " {\n" + " throw err;\n" + " }\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " try\n" + " {\n" + " foo();\n" + " }\n" + " catch(exception* err)\n" + " {\n" + " throw err;\n" + " }\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " try\n" + " {\n" + " foo();\n" + " }\n" + " catch(const exception& err)\n" + " {\n" + " throw err;\n" + " }\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " try\n" + " {\n" + " foo();\n" + " }\n" + " catch(int err)\n" + " {\n" + " throw err;\n" + " }\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " try\n" + " {\n" + " foo();\n" + " }\n" + " catch(exception* const err)\n" + " {\n" + " throw err;\n" + " }\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)