From f843678a077ae7153e7993af87e7fb2541186894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 16 Sep 2010 18:49:23 +0200 Subject: [PATCH] Redundant conditions: some refactorings * removed the 'redundant null pointer' check. sometimes it's unsafe to delete NULL pointer. and this check doesn't point out errors anyway. * moved the 'redundant condition' check for set::remove. Moved it to CheckStl. --- Makefile | 4 - lib/checkother.cpp | 155 --------------------------------------- lib/checkother.h | 13 ---- lib/checkstl.cpp | 37 ++++++++++ lib/checkstl.h | 11 ++- test/test.pro | 1 - test/testother.cpp | 135 ---------------------------------- test/testredundantif.cpp | 87 ---------------------- test/teststl.cpp | 27 +++++++ 9 files changed, 74 insertions(+), 396 deletions(-) delete mode 100644 test/testredundantif.cpp diff --git a/Makefile b/Makefile index 3550ef4a3..f85046cad 100644 --- a/Makefile +++ b/Makefile @@ -54,7 +54,6 @@ TESTOBJ = test/testautovariables.o \ test/testobsoletefunctions.o \ test/testother.o \ test/testpreprocessor.o \ - test/testredundantif.o \ test/testrunner.o \ test/testsettings.o \ test/testsimplifytokens.o \ @@ -227,9 +226,6 @@ test/testother.o: test/testother.cpp lib/tokenize.h lib/classinfo.h lib/token.h test/testpreprocessor.o: test/testpreprocessor.cpp test/testsuite.h lib/errorlogger.h lib/preprocessor.h lib/tokenize.h lib/classinfo.h lib/token.h lib/settings.h $(CXX) $(CXXFLAGS) -Ilib -Icli -c -o test/testpreprocessor.o test/testpreprocessor.cpp -test/testredundantif.o: test/testredundantif.cpp lib/tokenize.h lib/classinfo.h lib/token.h lib/checkother.h lib/check.h lib/settings.h lib/errorlogger.h test/testsuite.h - $(CXX) $(CXXFLAGS) -Ilib -Icli -c -o test/testredundantif.o test/testredundantif.cpp - test/testrunner.o: test/testrunner.cpp test/testsuite.h lib/errorlogger.h $(CXX) $(CXXFLAGS) -Ilib -Icli -c -o test/testrunner.o test/testrunner.cpp diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 927f2a758..abc4c82cc 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -73,151 +73,6 @@ void CheckOther::warningOldStylePointerCast() - -//--------------------------------------------------------------------------- -// Redundant code.. -//--------------------------------------------------------------------------- - -void CheckOther::warningRedundantCode() -{ - if (!_settings->_checkCodingStyle) - return; - - // if (p) delete p - for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) - { - if (! Token::simpleMatch(tok, "if (")) - continue; - - const Token *tok2 = tok->tokAt(2); - - /* - * Possible if-constructions: - * - * if (var) - * if (this->var) - * if (Foo::var) - * - **/ - std::string varname = concatNames(&tok2); - - if (!Token::Match(tok2, "%var% ) {")) - continue; - - tok2 = tok2->tokAt(3); - - /* - * Possible constructions: - * - * - delete %var% - * - delete [] %var% - * - free ( %var ) - * - kfree ( %var% ) - * - * Where %var% may be: - * - just variable name (var) - * - class member (this->var) - * - static member (Class::var) - * - **/ - - bool funcHasBracket = false; - if (Token::Match(tok2, "free|kfree (")) - { - tok2 = tok2->tokAt(2); - funcHasBracket = true; - - } - else if (tok2->str() == "delete") - { - - tok2 = tok2->next(); - - if (Token::simpleMatch(tok2, "[ ]")) - { - tok2 = tok2->tokAt(2); - } - } - - std::string varname2 = concatNames(&tok2); - - if (Token::Match(tok2, "%var%") && varname == varname2) - tok2 = tok2->next(); - else - continue; - - if (funcHasBracket) - { - if (tok2->str() != ")") - { - continue; - } - else - { - tok2 = tok2->next(); - } - } - - /* - * Possible constructions: - * - * - if (%var%) { delete %var%; } - * - if (%var%) { delete %var%; %var% = 0; } - * - **/ - if (Token::Match(tok2, "; } !!else")) - { - redundantIfDelete0Error(tok); - } - else if (Token::Match(tok2, "; %var%")) - { - tok2 = tok2->next(); - std::string varname3 = concatNames(&tok2); - if (Token::Match(tok2, "%var% = 0 ; } !!else") && varname2 == varname3) - { - redundantIfDelete0Error(tok); - } - } - } - - - - // Redundant condition - // if (haystack.find(needle) != haystack.end()) - // haystack.remove(needle); - redundantCondition2(); -} -//--------------------------------------------------------------------------- - -void CheckOther::redundantCondition2() -{ - const char pattern[] = "if ( %var% . find ( %any% ) != %var% . end ( ) ) " - "{|{|" - " %var% . remove ( %any% ) ; " - "}|}|"; - const Token *tok = Token::findmatch(_tokenizer->tokens(), pattern); - while (tok) - { - bool b(tok->tokAt(15)->str() == "{"); - - // Get tokens for the fields %var% and %any% - const Token *var1 = tok->tokAt(2); - const Token *any1 = tok->tokAt(6); - const Token *var2 = tok->tokAt(9); - const Token *var3 = tok->tokAt(b ? 16 : 15); - const Token *any2 = tok->tokAt(b ? 20 : 19); - - // Check if all the "%var%" fields are the same and if all the "%any%" are the same.. - if (var1->str() == var2->str() && - var2->str() == var3->str() && - any1->str() == any2->str()) - { - redundantIfRemoveError(tok); - } - - tok = Token::findmatch(tok->next(), pattern); - } -} //--------------------------------------------------------------------------- // "if (strlen(s))" can be rewritten as "if (*s != '\0')" //--------------------------------------------------------------------------- @@ -4045,16 +3900,6 @@ void CheckOther::cstyleCastError(const Token *tok) reportError(tok, Severity::style, "cstyleCast", "C-style pointer casting"); } -void CheckOther::redundantIfDelete0Error(const Token *tok) -{ - reportError(tok, Severity::style, "redundantIfDelete0", "Redundant condition. It is safe to deallocate a NULL pointer"); -} - -void CheckOther::redundantIfRemoveError(const Token *tok) -{ - reportError(tok, Severity::style, "redundantIfRemove", "Redundant condition. The remove function in the STL will not do anything if element doesn't exist"); -} - void CheckOther::dangerousUsageStrtolError(const Token *tok) { reportError(tok, Severity::error, "dangerousUsageStrtol", "Invalid radix in call to strtol or strtoul. Must be 0 or 2-36"); diff --git a/lib/checkother.h b/lib/checkother.h index 82b1d9743..9d9bb0aa0 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -71,7 +71,6 @@ public: CheckOther checkOther(tokenizer, settings, errorLogger); // Coding style checks - checkOther.warningRedundantCode(); checkOther.checkConstantFunctionParameter(); checkOther.checkIncompleteStatement(); checkOther.checkEmptyStringTest(); @@ -104,9 +103,6 @@ public: /** @brief Are there C-style pointer casts in a c++ file? */ void warningOldStylePointerCast(); - /** @brief Redundant code: if (p) delete p; */ - void warningRedundantCode(); - /** * @brief Invalid function usage (invalid radix / overlapping data) * @@ -163,11 +159,6 @@ public: void lookupVar(const Token *tok1, const std::string &varname); - // Redundant condition - // if (haystack.find(needle) != haystack.end()) - // haystack.remove(needle); - void redundantCondition2(); - /** @brief %Check for inefficient empty string test*/ void checkEmptyStringTest(); @@ -194,8 +185,6 @@ public: // Error messages.. void cstyleCastError(const Token *tok); - void redundantIfDelete0Error(const Token *tok); - void redundantIfRemoveError(const Token *tok); void dangerousUsageStrtolError(const Token *tok); void sprintfOverlappingDataError(const Token *tok, const std::string &varname); void udivError(const Token *tok); @@ -236,8 +225,6 @@ public: // style cstyleCastError(0); - redundantIfDelete0Error(0); - redundantIfRemoveError(0); dangerousUsageStrtolError(0); unusedStructMemberError(0, "structname", "variable"); passedByValueError(0, "parametername"); diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index a8bac389b..ad40c6265 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -711,3 +711,40 @@ void CheckStl::sizeError(const Token *tok) const bool verbose(_settings ? _settings->_verbose : true); reportError(tok, Severity::style, "stlSize", "Use " + varname + ".empty() instead of " + varname + ".size() to guarantee fast code." + (verbose ? " size() can take linear time but empty() is guaranteed to take constant time." : "")); } + + + +void CheckStl::redundantCondition() +{ + const char pattern[] = "if ( %var% . find ( %any% ) != %var% . end ( ) ) " + "{|{|" + " %var% . remove ( %any% ) ; " + "}|}|"; + const Token *tok = Token::findmatch(_tokenizer->tokens(), pattern); + while (tok) + { + bool b(tok->tokAt(15)->str() == "{"); + + // Get tokens for the fields %var% and %any% + const Token *var1 = tok->tokAt(2); + const Token *any1 = tok->tokAt(6); + const Token *var2 = tok->tokAt(9); + const Token *var3 = tok->tokAt(b ? 16 : 15); + const Token *any2 = tok->tokAt(b ? 20 : 19); + + // Check if all the "%var%" fields are the same and if all the "%any%" are the same.. + if (var1->str() == var2->str() && + var2->str() == var3->str() && + any1->str() == any2->str()) + { + redundantIfRemoveError(tok); + } + + tok = Token::findmatch(tok->next(), pattern); + } +} + +void CheckStl::redundantIfRemoveError(const Token *tok) +{ + reportError(tok, Severity::style, "redundantIfRemove", "Redundant condition. The remove function in the STL will not do anything if element doesn't exist"); +} diff --git a/lib/checkstl.h b/lib/checkstl.h index 2de554d6c..bd4f91aa7 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -57,6 +57,7 @@ public: // Style check checkStl.size(); + checkStl.redundantCondition(); } @@ -105,6 +106,11 @@ public: */ void size(); + /** + * Check for redundant condition 'if (ints.find(1) != ints.end()) ints.remove(123);' + * */ + void redundantCondition(); + private: /** @@ -125,6 +131,7 @@ private: void if_findError(const Token *tok, bool str); void sizeError(const Token *tok); void eraseByValueError(const Token *tok, const std::string &containername, const std::string &itername); + void redundantIfRemoveError(const Token *tok); void getErrorMessages() { @@ -141,6 +148,7 @@ private: if_findError(0, true); sizeError(0); eraseByValueError(0, "container", "iterator"); + redundantIfRemoveError(0); } std::string name() const @@ -157,7 +165,8 @@ private: "* dereferencing an erased iterator\n" "* for vectors: using iterator/pointer after push_back has been used\n" "* optimisation: use empty() instead of size() to guarantee fast code\n" - "* suspicious condition when using find\n"; + "* suspicious condition when using find\n" + "* redundant condition\n"; } bool isStlContainer(const Token *tok); diff --git a/test/test.pro b/test/test.pro index c4fe4e592..e5fae540b 100644 --- a/test/test.pro +++ b/test/test.pro @@ -24,7 +24,6 @@ SOURCES += testautovariables.cpp \ testmemleak.cpp \ testother.cpp \ testpreprocessor.cpp \ - testredundantif.cpp \ testrunner.cpp \ testsimplifytokens.cpp \ teststl.cpp \ diff --git a/test/testother.cpp b/test/testother.cpp index 3b85d07df..58a33b8af 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -39,9 +39,6 @@ private: TEST_CASE(zeroDiv3); TEST_CASE(zeroDiv4); - TEST_CASE(delete1); - TEST_CASE(delete2); - TEST_CASE(sprintf1); // Dangerous usage of sprintf TEST_CASE(sprintf2); TEST_CASE(sprintf3); @@ -130,7 +127,6 @@ private: // Simplify token list.. tokenizer.simplifyTokenList(); - checkOther.warningRedundantCode(); checkOther.checkZeroDivision(); checkOther.checkMathFunctions(); checkOther.checkEmptyStringTest(); @@ -249,137 +245,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void delete1() - { - check("void foo()\n" - "{\n" - " if (p)\n" - " {\n" - " delete p;\n" - " z = 0;\n" - " }\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check("void foo()\n" - "{\n" - " if (p)\n" - " delete p;\n" - " else\n" - " p = new P();\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check("void foo()\n" - "{\n" - " if (0 != g->p)\n" - " delete this->p;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check("void foo()\n" - "{\n" - " if (0 != g->p)\n" - " {\n" - " delete g->p;\n" - " g->z = 0;\n" - " }\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - - check("void foo()\n" - "{\n" - " if (0 != this->g->a)\n" - " delete this->p->a;\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); - } - - void delete2() - { - check("void foo()\n" - "{\n" - " if (p)\n" - " {\n" - " delete p;\n" - " }\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - check("void foo()\n" - "{\n" - " if (p)\n" - " {\n" - " delete p;\n" - " p = 0;\n" - " }\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - check("void foo()\n" - "{\n" - " if (p)\n" - " delete p;\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - check("void foo()\n" - "{\n" - " if (p != NULL)\n" - " delete p;\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - check("void foo()\n" - "{\n" - " if (p != NULL)\n" - " {\n" - " delete p;\n" - " p = NULL;\n" - " }\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - check("void foo()\n" - "{\n" - " if (p)\n" - " delete [] p;\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - check("void foo()\n" - "{\n" - " if (0 != this->p)\n" - " delete this->p;\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - check("void foo()\n" - "{\n" - " if (0 != this->p->a)\n" - " delete this->p->a;\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - - check("void Foo::deleteInstance()\n" - "{\n" - " if (Foo::instance != NULL)\n" - " delete Foo::instance;\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - - check("void Foo::deleteInstance()\n" - "{\n" - " if (Foo::instance != NULL)\n" - " {\n" - " delete Foo::instance;\n" - " Foo::instance = NULL;\n" - " }\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. It is safe to deallocate a NULL pointer\n", errout.str()); - } - void sprintfUsage(const char code[]) { diff --git a/test/testredundantif.cpp b/test/testredundantif.cpp deleted file mode 100644 index b4d9bde61..000000000 --- a/test/testredundantif.cpp +++ /dev/null @@ -1,87 +0,0 @@ -/* - * Cppcheck - A tool for static C/C++ code analysis - * Copyright (C) 2007-2010 Daniel Marjamäki and Cppcheck team. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ - - -// Check for dangerous division.. -// such as "svar / uvar". Treating "svar" as unsigned data is not good - - -#include "tokenize.h" -#include "checkother.h" -#include "testsuite.h" - -#include - -extern std::ostringstream errout; - -class TestRedundantIf : public TestFixture -{ -public: - TestRedundantIf() : TestFixture("TestRedundantIf") - { } - -private: - void check(const char code[]) - { - // Tokenize.. - Tokenizer tokenizer; - std::istringstream istr(code); - tokenizer.tokenize(istr, "test.cpp"); - - // Clear the error buffer.. - errout.str(""); - - // Check for redundant condition.. - Settings settings; - CheckOther checkOther(&tokenizer, &settings, this); - checkOther.redundantCondition2(); - } - - void run() - { - TEST_CASE(remove1); - TEST_CASE(remove2); - } - - void remove1() - { - check("void f()\n" - "{\n" - " if (haystack.find(needle) != haystack.end())\n" - " haystack.remove(needle);" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. The remove function in the STL will not do anything if element doesn't exist\n", errout.str()); - } - - void remove2() - { - check("void f()\n" - "{\n" - " if (haystack.find(needle) != haystack.end())\n" - " {\n" - " haystack.remove(needle);\n" - " }\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. The remove function in the STL will not do anything if element doesn't exist\n", errout.str()); - } - -}; - -REGISTER_TEST(TestRedundantIf) - - diff --git a/test/teststl.cpp b/test/teststl.cpp index 40fb31a7f..bbb2996fd 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -86,6 +86,11 @@ private: TEST_CASE(if_str_find); TEST_CASE(size1); + + // Redundant conditions.. + // if (ints.find(123) != ints.end()) ints.remove(123); + TEST_CASE(redundantCondition1); + TEST_CASE(redundantCondition2); } void check(const std::string &code) @@ -946,6 +951,28 @@ private: ASSERT_EQUALS("", errout.str()); } + void redundantCondition1() + { + check("void f()\n" + "{\n" + " if (haystack.find(needle) != haystack.end())\n" + " haystack.remove(needle);" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. The remove function in the STL will not do anything if element doesn't exist\n", errout.str()); + } + + void redundantCondition2() + { + check("void f()\n" + "{\n" + " if (haystack.find(needle) != haystack.end())\n" + " {\n" + " haystack.remove(needle);\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Redundant condition. The remove function in the STL will not do anything if element doesn't exist\n", errout.str()); + } + }; REGISTER_TEST(TestStl)