walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

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.
This commit is contained in:
Daniel Marjamäki 2010-09-16 18:49:23 +02:00
parent a56f6d276a
commit f843678a07
9 changed files with 74 additions and 396 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
Makefile
View File

@ -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

155
lib/checkother.cpp
View File

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

13
lib/checkother.h
View File

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

37
lib/checkstl.cpp
View File

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

11
lib/checkstl.h
View File

@ -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);

1
test/test.pro
View File

@ -24,7 +24,6 @@ SOURCES += testautovariables.cpp \
testmemleak.cpp \
testother.cpp \
testpreprocessor.cpp \
testredundantif.cpp \
testrunner.cpp \
testsimplifytokens.cpp \
teststl.cpp \

135
test/testother.cpp
View File

@ -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[])
{

87
test/testredundantif.cpp
View File

@ -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 <http://www.gnu.org/licenses/>.
*/
// 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 <sstream>
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)

27
test/teststl.cpp
View File

@ -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)