- Moved checkCatchExceptionByValue from CheckOther to CheckExceptionSafety

- Fixed false positive: throw outerCatchVar; in inner catch is now correctly handled
- Added eTry and eCatch to Scope::isLocal -> Scopes inside catch are now detected by symbol database

lib/checkexceptionsafety.cpp

@@ -144,9 +144,34 @@ void CheckExceptionSafety::checkRethrowCopy()
 
         const unsigned int varid = i->classStart->tokAt(-2)->varId();
         if (varid) {
-            const Token* rethrowTok = Token::findmatch(i->classStart->next(), "throw %varid% ;", i->classEnd->previous(), varid);
+            for (const Token* tok = i->classStart->next(); tok && tok != i->classEnd; tok = tok->next()) {
-            if (rethrowTok)
+                if (Token::simpleMatch(tok, "catch (") && tok->next()->link() && tok->next()->link()->next()) // Don't check inner catch - it is handled in another iteration of outer loop.
-                rethrowCopyError(rethrowTok, rethrowTok->strAt(1));
+                    tok = tok->next()->link()->next()->link();
+                else if (Token::Match(tok, "throw %varid% ;", varid))
+                    rethrowCopyError(tok, tok->strAt(1));
+            }
         }
     }
 }
+
+//---------------------------------------------------------------------------
+//    try {} catch (std::exception err) {} <- Should be "std::exception& err"
+//---------------------------------------------------------------------------
+void CheckExceptionSafety::checkCatchExceptionByValue()
+{
+    if (!_settings->isEnabled("style"))
+        return;
+
+    const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase();
+
+    for (std::list<Scope>::const_iterator i = symbolDatabase->scopeList.begin(); i != symbolDatabase->scopeList.end(); ++i) {
+        if (i->type != Scope::eCatch)
+            continue;
+
+        // Find a pass-by-value declaration in the catch(), excluding basic types
+        // e.g. catch (std::exception err)
+        const Variable* var = symbolDatabase->getVariableFromVarId(i->classStart->tokAt(-2)->varId());
+        if (var && var->isClass() && !var->isPointer() && !var->isReference())
+            catchExceptionByValueError(i->classDef);
+    }
+}

lib/checkexceptionsafety.h

@@ -56,6 +56,7 @@ public:
         checkExceptionSafety.destructors();
         checkExceptionSafety.deallocThrow();
         checkExceptionSafety.checkRethrowCopy();
+        checkExceptionSafety.checkCatchExceptionByValue();
     }
 
     /** Don't throw exceptions in destructors */
@@ -67,6 +68,9 @@ public:
     /** Don't rethrow a copy of the caught exception; use a bare throw instead */
     void checkRethrowCopy();
 
+    /** @brief %Check for exceptions that are caught by value instead of by reference */
+    void checkCatchExceptionByValue();
+
 private:
     /** Don't throw exceptions in destructors */
     void destructorsError(const Token * const tok) {
@@ -84,12 +88,20 @@ private:
                     "To rethrow the caught exception without unnecessary copying or slicing, use a bare 'throw;'.");
     }
 
+    void catchExceptionByValueError(const Token *tok) {
+        reportError(tok, Severity::style,
+                    "catchExceptionByValue", "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++.");
+    }
+
     /** Generate all possible errors (for --errorlist) */
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) {
         CheckExceptionSafety c(0, settings, errorLogger);
         c.destructorsError(0);
         c.deallocThrowError(0, "p");
         c.rethrowCopyError(0, "varname");
+        c.catchExceptionByValueError(0);
     }
 
     /** Short description of class (for --doc) */
@@ -102,7 +114,8 @@ private:
         return "Checking exception safety\n"
                "* Throwing exceptions in destructors\n"
                "* Throwing exception during invalid state\n"
-               "* Throwing a copy of a caught exception instead of rethrowing the original exception";
+               "* Throwing a copy of a caught exception instead of rethrowing the original exception\n"
+               "* exception caught by value instead of by reference";
     }
 };
 /// @}

lib/checkother.cpp

@@ -1058,36 +1058,6 @@ void CheckOther::secondAlwaysTrueFalseWhenFirstTrueError(const Token *tok, const
     reportError(tok, Severity::style, "secondAlwaysTrueFalseWhenFirstTrue", truefalse);
 }
 
-//---------------------------------------------------------------------------
-//    try {} catch (std::exception err) {} <- Should be "std::exception& err"
-//---------------------------------------------------------------------------
-void CheckOther::checkCatchExceptionByValue()
-{
-    if (!_settings->isEnabled("style"))
-        return;
-
-    const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase();
-
-    for (std::list<Scope>::const_iterator i = symbolDatabase->scopeList.begin(); i != symbolDatabase->scopeList.end(); ++i) {
-        if (i->type != Scope::eCatch)
-            continue;
-
-        // Find a pass-by-value declaration in the catch(), excluding basic types
-        // e.g. catch (std::exception err)
-        const Variable* var = symbolDatabase->getVariableFromVarId(i->classStart->tokAt(-2)->varId());
-        if (var && var->isClass() && !var->isPointer() && !var->isReference())
-            catchExceptionByValueError(i->classDef);
-    }
-}
-
-void CheckOther::catchExceptionByValueError(const Token *tok)
-{
-    reportError(tok, Severity::style,
-                "catchExceptionByValue", "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++.");
-}
-
 //---------------------------------------------------------------------------
 // strtol(str, 0, radix)  <- radix must be 0 or 2-36
 //---------------------------------------------------------------------------

lib/checkother.h

@@ -96,7 +96,6 @@ public:
         checkOther.checkCoutCerrMisusage();
         checkOther.checkIncorrectLogicOperator();
         checkOther.checkMisusedScopedObject();
-        checkOther.checkCatchExceptionByValue();
         checkOther.checkMemsetZeroBytes();
         checkOther.checkIncorrectStringCompare();
         checkOther.checkIncrementBoolean();
@@ -208,9 +207,6 @@ 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();
-
     /** @brief %Check for filling zero bytes with memset() */
     void checkMemsetZeroBytes();
 
@@ -289,7 +285,6 @@ public:
     void incorrectLogicOperatorError(const Token *tok, bool always);
     void secondAlwaysTrueFalseWhenFirstTrueError(const Token *tok, const std::string &truefalse);
     void misusedScopeObjectError(const Token *tok, const std::string &varname);
-    void catchExceptionByValueError(const Token *tok);
     void memsetZeroBytesError(const Token *tok, const std::string &varname);
     void sizeofForArrayParameterError(const Token *tok);
     void sizeofForStrncmpError(const Token *tok);
@@ -349,7 +344,6 @@ public:
         c.invalidScanfError(0);
         c.incorrectLogicOperatorError(0, true);
         c.secondAlwaysTrueFalseWhenFirstTrueError(0, "when first comparison is true, the 2nd comparison is always true");
-        c.catchExceptionByValueError(0);
         c.memsetZeroBytesError(0, "varname");
         c.clarifyCalculationError(0, "+");
         c.clarifyConditionError(0, true, false);
@@ -418,7 +412,6 @@ 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"
                "* Clarify calculation with parentheses\n"
                "* using increment on boolean\n"
                "* comparison of a boolean with a non-zero integer\n"

lib/symboldatabase.h

@@ -459,7 +459,8 @@ public:
     bool isLocal() const {
         return (type == eIf || type == eElse || type == eElseIf ||
                 type == eFor || type == eWhile || type == eDo ||
-                type == eSwitch || type == eUnconditional);
+                type == eSwitch || type == eUnconditional ||
+                type == eTry || type == eCatch);
     }
 
     /**

test/testexceptionsafety.cpp

@@ -40,6 +40,8 @@ private:
         TEST_CASE(rethrowCopy2);
         TEST_CASE(rethrowCopy3);
         TEST_CASE(rethrowCopy4);
+        TEST_CASE(rethrowCopy5);
+        TEST_CASE(catchExceptionByValue);
     }
 
     void check(const std::string &code, bool inconclusive = false) {
@@ -157,7 +159,7 @@ private:
               "    {\n"
               "       foo();\n"
               "    }\n"
-              "    catch(exception err)\n"
+              "    catch(exception& err)\n"
               "    {\n"
               "        throw err;\n"
               "    }\n"
@@ -170,7 +172,7 @@ private:
               "    try {\n"
               "       foo();\n"
               "    }\n"
-              "    catch(std::runtime_error err) {\n"
+              "    catch(std::runtime_error& err) {\n"
               "        throw err;\n"
               "    }\n"
               "}");
@@ -183,7 +185,7 @@ private:
               "    {\n"
               "       foo();\n"
               "    }\n"
-              "    catch(exception err)\n"
+              "    catch(const exception& err)\n"
               "    {\n"
               "        exception err2;\n"
               "        throw err2;\n"
@@ -191,7 +193,110 @@ private:
               "}\n");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void rethrowCopy5() {
+        check("void f() {\n"
+              "    try {\n"
+              "       foo();\n"
+              "    }\n"
+              "    catch(const exception& outer) {\n"
+              "        try {\n"
+              "           foo(outer);\n"
+              "        }\n"
+              "        catch(const exception& inner) {\n"
+              "            throw inner;\n"
+              "        }\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:10]: (style) Throwing a copy of the caught exception instead of rethrowing the original exception\n", errout.str());
+
+        check("void f() {\n"
+              "    try {\n"
+              "       foo();\n"
+              "    }\n"
+              "    catch(const exception& outer) {\n"
+              "        try {\n"
+              "           foo(outer);\n"
+              "        }\n"
+              "        catch(const exception& inner) {\n"
+              "            throw outer;\n"
+              "        }\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+    }
+
+    void catchExceptionByValue() {
+        check("void f() {\n"
+              "    try {\n"
+              "        bar();\n"
+              "    }\n"
+              "    catch( ::std::exception err) {\n"
+              "        foo(err);\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:5]: (style) Exception should be caught by reference.\n", errout.str());
+
+        check("void f() {\n"
+              "    try {\n"
+              "        bar();\n"
+              "    }\n"
+              "    catch(const exception err) {\n"
+              "        foo(err);\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:5]: (style) Exception should be caught by reference.\n", errout.str());
+
+        check("void f() {\n"
+              "    try {\n"
+              "        bar();\n"
+              "    }\n"
+              "    catch( ::std::exception& err) {\n"
+              "        foo(err);\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    try {\n"
+              "        bar();\n"
+              "    }\n"
+              "    catch(exception* err) {\n"
+              "        foo(err);\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    try {\n"
+              "        bar();\n"
+              "    }\n"
+              "    catch(const exception& err) {\n"
+              "        foo(err);\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    try {\n"
+              "        bar();\n"
+              "    }\n"
+              "    catch(int err) {\n"
+              "        foo(err);\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    try {\n"
+              "        bar();\n"
+              "    }\n"
+              "    catch(exception* const err) {\n"
+              "        foo(err);\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestExceptionSafety)
-

test/testother.cpp

@@ -114,8 +114,6 @@ private:
         TEST_CASE(incorrectLogicOperator2);
         TEST_CASE(secondAlwaysTrueFalseWhenFirstTrueError);
 
-        TEST_CASE(catchExceptionByValue);
-
         TEST_CASE(memsetZeroBytes);
 
         TEST_CASE(sizeofForArrayParameter);
@@ -3065,99 +3063,6 @@ private:
     }
 
 
-    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());
-    }
-
     void memsetZeroBytes() {
         check("void f() {\n"
               "    memset(p, 10, 0x0);\n"