From 3a8e7b4bf0242f7bcc1fbea9f0f4f83aef0602d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 31 Aug 2010 22:22:59 +0200 Subject: [PATCH] Exception safety: Removed the noisy checks and keep the useful checks --- lib/checkexceptionsafety.cpp | 210 ----------------------------------- lib/checkexceptionsafety.h | 24 ---- lib/cppcheck.cpp | 2 - lib/settings.cpp | 2 - test/testexceptionsafety.cpp | 115 ------------------- 5 files changed, 353 deletions(-) diff --git a/lib/checkexceptionsafety.cpp b/lib/checkexceptionsafety.cpp index ef41e998e..902b226b4 100644 --- a/lib/checkexceptionsafety.cpp +++ b/lib/checkexceptionsafety.cpp @@ -73,216 +73,6 @@ void CheckExceptionSafety::destructors() } -void CheckExceptionSafety::unsafeNew() -{ - if (!_settings->isEnabled("exceptNew")) - return; - - // Inspect initializer lists.. - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) - { - if (tok->str() != ")") - continue; - tok = tok->next(); - if (!tok) - break; - if (tok->str() != ":") - continue; - - // multiple "new" in an initializer list.. - std::string varname; - for (tok = tok->next(); tok; tok = tok->next()) - { - if (!Token::Match(tok, "%var% (")) - break; - tok = tok->next(); - if (Token::Match(tok->next(), "new %type%")) - { - if (!varname.empty()) - { - unsafeNewError(tok->previous(), varname); - break; - } - /* - // TODO: check if class is autodeallocated+might throw exception in constructor - if (!_settings->isAutoDealloc(tok->strAt(2))) - { - varname = tok->strAt(-1); - } - */ - } - tok = tok->link(); - tok = tok ? tok->next() : 0; - if (!tok) - break; - if (tok->str() != ",") - break; - } - if (!tok) - break; - } - - - // Inspect constructors.. - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) - { - // match this pattern.. "C :: C ( .. ) {" - if (!tok->next() || tok->next()->str() != "::") - continue; - if (!Token::Match(tok, "%var% :: %var% (")) - continue; - if (tok->str() != tok->strAt(2)) - continue; - if (!Token::simpleMatch(tok->tokAt(3)->link(), ") {")) - continue; - - // inspect the constructor.. - std::string varname; - for (tok = tok->tokAt(3)->link()->tokAt(2); tok; tok = tok->next()) - { - if (tok->str() == "{" || tok->str() == "}") - break; - // some variable declaration.. - if (Token::Match(tok->previous(), "[{;] %type% * %var% ;")) - break; - // allocating with new.. - if (Token::Match(tok, "%var% = new %type%")) - { - if (!varname.empty()) - { - unsafeNewError(tok, varname); - break; - } - /* - TODO: check is class is autodeallocated + might throw exceptions in the constructor - if (!_settings->isAutoDealloc(tok->strAt(3))) - varname = tok->str(); - */ - } - } - } - - // allocating multiple local variables.. - std::set localVars; - std::string varname; - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) - { - if (tok->str() == "{" || tok->str() == "}") - { - localVars.clear(); - varname = ""; - } - - if (Token::Match(tok, "[;{}] %type% * %var% ;") && - tok->tokAt(1)->isStandardType()) - { - tok = tok->tokAt(3); - if (tok->varId()) - localVars.insert(tok->varId()); - } - - if (Token::Match(tok, "[;{}] %var% = new %type%")) - { - if (!varname.empty()) - { - unsafeNewError(tok->next(), varname); - break; - } - if (tok->next()->varId() && localVars.find(tok->next()->varId()) != localVars.end()) - varname = tok->strAt(1); - } - - else if (Token::Match(tok, "[;{}] %var% (")) - { - // False negatives: we don't handle switch cases properly so we just bail out. - if (tok->strAt(1) == "switch") - break; - - for (tok = tok->next(); tok && !Token::Match(tok, "[;{}]"); tok = tok->next()) - { - if (tok->str() == varname) - { - varname = ""; - } - } - if (!tok) - break; - } - - else if (tok->str() == "delete") - { - if (Token::simpleMatch(tok->next(), varname.c_str()) || - Token::simpleMatch(tok->next(), ("[ ] " + varname).c_str())) - { - varname = ""; - } - } - } -} - - -void CheckExceptionSafety::realloc() -{ - if (!_settings->isEnabled("exceptRealloc")) - return; - - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) - { - if (Token::simpleMatch(tok, "try {")) - { - tok = tok->next()->link(); - if (!tok) - break; - } - - if (!Token::Match(tok, "[{};] delete")) - continue; - - tok = tok->tokAt(2); - if (Token::simpleMatch(tok, "[ ]")) - tok = tok->tokAt(2); - if (!tok) - break; - - // reallocating.. - if (!Token::Match(tok, "%var% ; %var% = new %type%")) - continue; - - // variable id of deallocated pointer.. - const unsigned int varid(tok->varId()); - if (varid == 0) - continue; - - // variable id of allocated pointer must match.. - if (varid != tok->tokAt(2)->varId()) - continue; - - // is is a class member variable.. - const Token *tok1 = Token::findmatch(_tokenizer->tokens(), "%varid%", varid); - while (0 != (tok1 = tok1 ? tok1->previous() : 0)) - { - if (tok1->str() == "}") - tok1 = tok1->link(); - else if (tok1->str() == "{") - { - if (tok1->previous() && tok1->previous()->isName()) - { - while (0 != (tok1 = tok1 ? tok1->previous() : 0)) - { - if (!tok1->isName() && tok1->str() != ":" && tok1->str() != ",") - break; - if (tok1->str() == "class") - { - reallocError(tok->tokAt(2), tok->str()); - break; - } - } - } - break; - } - } - } -} void CheckExceptionSafety::deallocThrow() diff --git a/lib/checkexceptionsafety.h b/lib/checkexceptionsafety.h index 12f800443..e969fe159 100644 --- a/lib/checkexceptionsafety.h +++ b/lib/checkexceptionsafety.h @@ -55,8 +55,6 @@ public: { CheckExceptionSafety checkExceptionSafety(tokenizer, settings, errorLogger); checkExceptionSafety.destructors(); - checkExceptionSafety.unsafeNew(); - checkExceptionSafety.realloc(); checkExceptionSafety.deallocThrow(); } @@ -64,12 +62,6 @@ public: /** Don't throw exceptions in destructors */ void destructors(); - /** unsafe use of "new" */ - void unsafeNew(); - - /** Unsafe realloc */ - void realloc(); - /** deallocating memory and then throw */ void deallocThrow(); @@ -80,18 +72,6 @@ private: reportError(tok, Severity::style, "exceptThrowInDestructor", "Throwing exception in destructor"); } - /** Unsafe use of new */ - void unsafeNewError(const Token * const tok, const std::string &varname) - { - reportError(tok, Severity::style, "exceptNew", "Upon exception there is memory leak: " + varname); - } - - /** Unsafe reallocation */ - void reallocError(const Token * const tok, const std::string &varname) - { - reportError(tok, Severity::style, "exceptRealloc", "Upon exception " + varname + " becomes a dead pointer"); - } - void deallocThrowError(const Token * const tok, const std::string &varname) { reportError(tok, Severity::error, "exceptDeallocThrow", "Throwing exception in invalid state, " + varname + " points at deallocated memory"); @@ -101,8 +81,6 @@ private: void getErrorMessages() { destructorsError(0); - unsafeNewError(0, "p"); - reallocError(0, "p"); deallocThrowError(0, "p"); } @@ -117,8 +95,6 @@ private: { return "Checking exception safety\n" "* Throwing exceptions in destructors\n" - "* Unsafe use of 'new'\n" - "* Unsafe reallocation\n" "* Throwing exception during invalid state"; } }; diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index 052bc456b..a8ecc5e72 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -602,8 +602,6 @@ bool CppCheck::parseFromArgs(int argc, const char* const argv[]) " Example: -DDEBUG=1 -D__cplusplus\n" " --enable=id Enable additional checks. The available ids are:\n" " * all - enable all checks\n" - " * exceptNew - exception safety when using new\n" - " * exceptRealloc - exception safety when reallocating\n" " * style - Check coding style\n" " * unusedFunctions - check for unused functions\n" " Several ids can be given if you separate them with commas\n" diff --git a/lib/settings.cpp b/lib/settings.cpp index 9d2b113f4..350945002 100644 --- a/lib/settings.cpp +++ b/lib/settings.cpp @@ -188,8 +188,6 @@ std::string Settings::addEnabled(const std::string &str) handled = _checkCodingStyle = true; std::set id; - id.insert("exceptNew"); - id.insert("exceptRealloc"); id.insert("unusedFunctions"); if (str == "all") diff --git a/test/testexceptionsafety.cpp b/test/testexceptionsafety.cpp index 7604178e9..0e1f72b22 100644 --- a/test/testexceptionsafety.cpp +++ b/test/testexceptionsafety.cpp @@ -35,9 +35,6 @@ private: void run() { TEST_CASE(destructors); - TEST_CASE(newnew); - TEST_CASE(switchnewnew); - TEST_CASE(realloc); TEST_CASE(deallocThrow); } @@ -68,118 +65,6 @@ private: ASSERT_EQUALS("[test.cpp:3]: (style) Throwing exception in destructor\n", errout.str()); } - void newnew() - { - check("C::C() : a(new A), b(new B) { }"); - ASSERT_EQUALS("", errout.str()); - TODO_ASSERT_EQUALS("[test.cpp:1]: (style) Upon exception there is memory leak: a\n", errout.str()); - - check("C::C()\n" - "{\n" - " a = new A;\n" - " b = new B;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - TODO_ASSERT_EQUALS("[test.cpp:4]: (style) Upon exception there is memory leak: a\n", errout.str()); - - check("void a()\n" - "{\n" - " A *a1 = new A;\n" - " A *a2 = new A;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - TODO_ASSERT_EQUALS("[test.cpp:4]: (style) Upon exception there is memory leak: a1\n", errout.str()); - - check("void a()\n" - "{\n" - " A *a1 = new A;\n" - " A *a2 = new (std::nothrow) A;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check("void a()\n" - "{\n" - " A *a1 = new A;\n" - " delete a1;\n" - " A *a2 = new A;\n" - " delete a2;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check("void a()\n" - "{\n" - " A *a = new A;\n" - " B *b = new B;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - // passing pointer to unknown function.. the pointer may be added to some list etc.. - check("void f()\n" - "{\n" - " A *a1 = new A;\n" - " add(a1);\n" - " A *a2 = new A;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - } - - void switchnewnew() - { - check("int *f(int x)\n" - "{\n" - " int *p = 0;\n" - " switch(x)\n" - " {\n" - " case 1:\n" - " p = new int(10);\n" - " break;\n" - " case 2:\n" - " p = new int(100);\n" - " break;\n" - " };\n" - " return p;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - } - - void realloc() - { - check("class A\n" - "{\n" - " int *p;\n" - " void a()\n" - " {\n" - " delete p;\n" - " p = new int[123];\n" - " }\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:7]: (style) Upon exception p becomes a dead pointer\n", errout.str()); - - check("class A\n" - "{\n" - " int *p;\n" - " void a()\n" - " {\n" - " delete p;\n" - " p = new (std::nothrow) int[123];\n" - " }\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check("class A\n" - "{\n" - " int *p;\n" - " void a()\n" - " {\n" - " try {\n" - " delete p;\n" - " p = new int[123];\n" - " } catch (...) { p = 0; }\n" - " }\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - } - void deallocThrow() { check("int * p;\n"