Fixed cppcheck-opensource#2388 (Use throw without argument to rethrow exceptions)

zblair · zblair · commit 8a494cf8bbc5 · 2011-02-05T01:11:09.000-08:00
diff --git a/lib/checkexceptionsafety.cpp b/lib/checkexceptionsafety.cpp
@@ -167,4 +167,32 @@ void CheckExceptionSafety::deallocThrow()
     }
 }
 
+//---------------------------------------------------------------------------
+//      catch(const exception & err)
+//      {
+//         throw err;            // <- should be just "throw;"
+//      }
+//---------------------------------------------------------------------------
+void CheckExceptionSafety::checkRethrowCopy()
+{
+    if (!_settings->_checkCodingStyle)
+        return;
+    const char catchPattern[] = "catch ( const| %type% &|*| %var% ) { %any%";
+
+    const Token *tok = Token::findmatch(_tokenizer->tokens(), catchPattern);
+    while (tok)
+    {
+        const Token *startBlockTok = tok->next()->link()->next();
+        const Token *endBlockTok = startBlockTok->link();
+        const unsigned int varid = startBlockTok->tokAt(-2)->varId();
+
+        const Token* rethrowTok = Token::findmatch(startBlockTok, "throw %varid%", endBlockTok, varid);
+        if (rethrowTok)
+        {
+            rethrowCopyError(rethrowTok);
+        }
+
+        tok = Token::findmatch(endBlockTok->next(), catchPattern);
+    }
+}
 
diff --git a/lib/checkexceptionsafety.h b/lib/checkexceptionsafety.h
@@ -57,6 +57,7 @@ class CheckExceptionSafety : public Check
         CheckExceptionSafety checkExceptionSafety(tokenizer, settings, errorLogger);
         checkExceptionSafety.destructors();
         checkExceptionSafety.deallocThrow();
+        checkExceptionSafety.checkRethrowCopy();
     }
 
     /** Don't throw exceptions in destructors */
@@ -65,6 +66,9 @@ class CheckExceptionSafety : public Check
     /** deallocating memory and then throw (dead pointer) */
     void deallocThrow();
 
+    /** Don't rethrow a copy of the caught exception; use a bare throw instead */
+    void checkRethrowCopy();
+
 private:
     /** Don't throw exceptions in destructors */
     void destructorsError(const Token * const tok)
@@ -77,12 +81,20 @@ class CheckExceptionSafety : public Check
         reportError(tok, Severity::error, "exceptDeallocThrow", "Throwing exception in invalid state, " + varname + " points at deallocated memory");
     }
 
+    void rethrowCopyError(const Token * const tok)
+    {
+        reportError(tok, Severity::style, "exceptRethrowCopy",
+                    "Throwing a copy of the caught exception instead of rethrowing the original exception\n"
+                    "To rethrow the caught exception without unnecessary copying or slicing, use a bare 'throw;'. ");
+    }
+
     /** 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);
     }
 
     /** Short description of class (for --doc) */
@@ -96,7 +108,8 @@ class CheckExceptionSafety : public Check
     {
         return "Checking exception safety\n"
                "* Throwing exceptions in destructors\n"
-               "* Throwing exception during invalid state";
+               "* Throwing exception during invalid state\n"
+               "* Throwing a copy of a caught exception instead of rethrowing the original exception";
     }
 };
 /// @}
diff --git a/test/testexceptionsafety.cpp b/test/testexceptionsafety.cpp
@@ -37,6 +37,9 @@ class TestExceptionSafety : public TestFixture
         TEST_CASE(destructors);
         TEST_CASE(deallocThrow1);
         TEST_CASE(deallocThrow2);
+        TEST_CASE(rethrowCopy1);
+        TEST_CASE(rethrowCopy2);
+        TEST_CASE(rethrowCopy3);
     }
 
     void check(const std::string &code)
@@ -90,6 +93,52 @@ class TestExceptionSafety : public TestFixture
               "}\n");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void rethrowCopy1()
+    {
+        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:8]: (style) Throwing a copy of the caught exception instead of rethrowing the original exception\n", errout.str());
+    }
+
+    void rethrowCopy2()
+    {
+        check("void f() {\n"
+              "    try\n"
+              "    {\n"
+              "       foo();\n"
+              "    }\n"
+              "    catch(exception err)\n"
+              "    {\n"
+              "        throw err;\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:8]: (style) Throwing a copy of the caught exception instead of rethrowing the original exception\n", errout.str());
+    }
+
+    void rethrowCopy3()
+    {
+        check("void f() {\n"
+              "    try\n"
+              "    {\n"
+              "       foo();\n"
+              "    }\n"
+              "    catch(exception err)\n"
+              "    {\n"
+              "        exception err2;\n"
+              "        throw err2;\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestExceptionSafety)
