Fixed #2382 (Catching exceptions by value instead of reference)

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++.");
+}

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";

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)