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

fix #311 (add detection of duplicated if else-cases)

This commit is contained in:
Robert Reif 2011-04-09 15:14:01 -04:00
parent 77aebd357e
commit 7e403ae210
3 changed files with 181 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

122
lib/checkother.cpp
View File

@ -3040,6 +3040,128 @@ void CheckOther::checkIncorrectStringCompare()
}
}
//-----------------------------------------------------------------------------
// check for duplicate expressions in if statements
// if (a) { } else if (a) { }
//-----------------------------------------------------------------------------
static const std::string stringifyTokens(const Token *start, const Token *end)
{
if (start == end)
return "";
const Token *tok = start;
std::string stringified = tok->str();
while (tok && tok->next() && tok != end)
{
tok = tok->next();
stringified.append(" ");
stringified.append(tok->str());
}
return stringified;
}
static bool expressionHasSideEffects(const Token *first, const Token *last)
{
for (const Token *tok = first; tok != last->next(); tok = tok->next())
{
// check for assignment
if (tok->isAssignmentOp())
return true;
// check for inc/dec
else if (Token::Match(tok, "++|--"))
return true;
// check for function call
else if (Token::Match(tok, "%var% (") &&
!(Token::Match(tok, "c_str|string") || tok->isStandardType()))
return true;
}
return false;
}
void CheckOther::checkDuplicateIf()
{
if (!_settings->_checkCodingStyle)
return;
const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
std::list<Scope>::const_iterator scope;
for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope)
{
// only check functions
if (scope->type != Scope::eFunction)
continue;
// check all the code in the function for if (...) and else if (...) statements
for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next())
{
if (Token::Match(tok, "if (") && tok->strAt(-1) != "else" &&
Token::Match(tok->next()->link(), ") {"))
{
std::map<std::string, const Token*> expressionMap;
// get the expression from the token stream
std::string expression = stringifyTokens(tok->tokAt(2), tok->next()->link()->previous());
// save the expression and its location
expressionMap.insert(std::make_pair(expression, tok));
// find the next else if (...) statement
const Token *tok1 = tok->next()->link()->next()->link();
// check all the else if (...) statements
while (Token::Match(tok1, "} else if (") &&
Token::Match(tok1->tokAt(3)->link(), ") {"))
{
// get the expression from the token stream
expression = stringifyTokens(tok1->tokAt(4), tok1->tokAt(3)->link()->previous());
// try to look up the expression to check for duplicates
std::map<std::string, const Token *>::iterator it = expressionMap.find(expression);
// found a duplicate
if (it != expressionMap.end())
{
// check for expressions that have side effects and ignore them
if (!expressionHasSideEffects(tok1->tokAt(4), tok1->tokAt(3)->link()->previous()))
duplicateIfError(it->second, tok1->next());
}
// not a duplicate expression so save it and its location
else
expressionMap.insert(std::make_pair(expression, tok1->next()));
// find the next else if (...) statement
tok1 = tok1->tokAt(3)->link()->next()->link();
}
tok = tok->next()->link()->next();
}
}
}
}
void CheckOther::duplicateIfError(const Token *tok1, const Token *tok2)
{
std::list<const Token *> toks;
toks.push_back(tok2);
toks.push_back(tok1);
reportError(toks, Severity::style, "duplicateIf", "Found duplicate if expressions.\n"
"Finding the same expression more than once is suspicious and might indicate "
"a cut and paste or logic error. Please examine this code carefully to determine "
"if it is correct.");
}
//-----------------------------------------------------------------------------
void CheckOther::cstyleCastError(const Token *tok)
{
reportError(tok, Severity::style, "cstyleCast", "C-style pointer casting");

10
lib/checkother.h
View File

@ -55,7 +55,6 @@ public:
checkOther.checkUnsignedDivision();
checkOther.checkCharVariable();
checkOther.functionVariableUsage();
checkOther.checkVariableScope();
checkOther.checkStructMemberUsage();
checkOther.strPlusChar();
checkOther.sizeofsizeof();
@ -64,6 +63,10 @@ public:
checkOther.checkAssignmentInAssert();
checkOther.checkSizeofForArrayParameter();
checkOther.checkSelfAssignment();
checkOther.checkDuplicateIf();
// information checks
checkOther.checkVariableScope();
checkOther.clarifyCondition(); // not simplified because ifAssign
}
@ -202,6 +205,9 @@ public:
/** @brief %Check for suspicious comparison of a bool and a non-zero (and non-one) value (e.g. "if (!x==4)") */
void checkComparisonOfBoolWithInt();
/** @brief %Check for suspicious code where multiple if have the same expression (e.g "if (a) { } else if (a) { }") */
void checkDuplicateIf();
// Error messages..
void cstyleCastError(const Token *tok);
void dangerousUsageStrtolError(const Token *tok);
@ -230,6 +236,7 @@ public:
void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string, const std::string &len);
void incrementBooleanError(const Token *tok);
void comparisonOfBoolWithIntError(const Token *tok, const std::string &varname);
void duplicateIfError(const Token *tok1, const Token *tok2);
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings)
{
@ -274,6 +281,7 @@ public:
c.incorrectStringCompareError(0, "substr", "\"Hello World\"", "12");
c.incrementBooleanError(0);
c.comparisonOfBoolWithIntError(0, "varname");
c.duplicateIfError(0, 0);
}
std::string myName() const

50
test/testother.cpp
View File

@ -112,6 +112,8 @@ private:
TEST_CASE(incrementBoolean);
TEST_CASE(comparisonOfBoolWithInt);
TEST_CASE(duplicateIf);
}
void check(const char code[], const char *filename = NULL)
@ -135,6 +137,7 @@ private:
checkOther.checkAssignmentInAssert();
checkOther.checkSizeofForArrayParameter();
checkOther.clarifyCondition();
checkOther.checkDuplicateIf();
// Simplify token list..
tokenizer.simplifyTokenList();
@ -2397,6 +2400,53 @@ private:
"}");
ASSERT_EQUALS("", errout.str());
}
void duplicateIf()
{
check("void f(int a, int &b) {\n"
" if (a == 1) { b = 1; }\n"
" else if (a == 2) { b = 2; }\n"
" else if (a == 1) { b = 3; }\n"
"}");
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2]: (style) Found duplicate if expressions.\n", errout.str());
check("void f(int a, int &b) {\n"
" if (a == 1) { b = 1; }\n"
" else if (a == 2) { b = 2; }\n"
" else if (a == 2) { b = 3; }\n"
"}");
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Found duplicate if expressions.\n", errout.str());
check("void f(int a, int &b) {\n"
" if (a == 1) {\n"
" b = 1;\n"
" if (b == 1) { }\n"
" else if (b == 1) { }\n"
" } else if (a == 2) { b = 2; }\n"
" else if (a == 2) { b = 3; }\n"
"}");
ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (style) Found duplicate if expressions.\n"
"[test.cpp:5] -> [test.cpp:4]: (style) Found duplicate if expressions.\n", errout.str());
check("void f(int a, int &b) {\n"
" if (a++) { b = 1; }\n"
" else if (a++) { b = 2; }\n"
" else if (a++) { b = 3; }\n"
"}");
ASSERT_EQUALS("", errout.str());
check("void f(int a, int &b) {\n"
" if (!strtok(NULL," ")) { b = 1; }\n"
" else if (!strtok(NULL," ")) { b = 2; }\n"
"}");
ASSERT_EQUALS("", errout.str());
check("void f(int a, int &b) {\n"
" if ((x = x / 2) < 100) { b = 1; }\n"
" else if ((x = x / 2) < 100) { b = 2; }\n"
"}");
ASSERT_EQUALS("", errout.str());
}
};
REGISTER_TEST(TestOther)