partial fix for #2700 (common logic or cut and paste errors)
This commit is contained in:
parent
d22fcb8184
commit
66de41b313
|
@ -3157,6 +3157,64 @@ void CheckOther::duplicateIfError(const Token *tok1, const Token *tok2)
|
||||||
"if it is correct.");
|
"if it is correct.");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
//-----------------------------------------------------------------------------
|
||||||
|
// check for duplicate code in if and else branches
|
||||||
|
// if (a) { b = true; } else { b = true; }
|
||||||
|
//-----------------------------------------------------------------------------
|
||||||
|
|
||||||
|
void CheckOther::checkDuplicateBranch()
|
||||||
|
{
|
||||||
|
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 (..) else
|
||||||
|
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(), ") {") &&
|
||||||
|
Token::Match(tok->next()->link()->next()->link(), "} else {"))
|
||||||
|
{
|
||||||
|
// save if branch code
|
||||||
|
std::string branch1 = stringifyTokens(tok->next()->link()->tokAt(2), tok->next()->link()->next()->link()->previous());
|
||||||
|
|
||||||
|
// find else branch
|
||||||
|
const Token *tok1 = tok->next()->link()->next()->link();
|
||||||
|
|
||||||
|
// save else branch code
|
||||||
|
std::string branch2 = stringifyTokens(tok1->tokAt(3), tok1->tokAt(2)->link()->previous());
|
||||||
|
|
||||||
|
// check for duplicates
|
||||||
|
if (branch1 == branch2)
|
||||||
|
duplicateBranchError(tok, tok1->tokAt(2));
|
||||||
|
|
||||||
|
tok = tok->next()->link()->next();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2)
|
||||||
|
{
|
||||||
|
std::list<const Token *> toks;
|
||||||
|
toks.push_back(tok2);
|
||||||
|
toks.push_back(tok1);
|
||||||
|
|
||||||
|
reportError(toks, Severity::style, "duplicateBranch", "Found duplicate branches for if and else.\n"
|
||||||
|
"Finding the same code for an if branch and an else branch 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)
|
void CheckOther::cstyleCastError(const Token *tok)
|
||||||
|
|
|
@ -64,6 +64,7 @@ public:
|
||||||
checkOther.checkSizeofForArrayParameter();
|
checkOther.checkSizeofForArrayParameter();
|
||||||
checkOther.checkSelfAssignment();
|
checkOther.checkSelfAssignment();
|
||||||
checkOther.checkDuplicateIf();
|
checkOther.checkDuplicateIf();
|
||||||
|
checkOther.checkDuplicateBranch();
|
||||||
|
|
||||||
// information checks
|
// information checks
|
||||||
checkOther.checkVariableScope();
|
checkOther.checkVariableScope();
|
||||||
|
@ -208,6 +209,9 @@ public:
|
||||||
/** @brief %Check for suspicious code where multiple if have the same expression (e.g "if (a) { } else if (a) { }") */
|
/** @brief %Check for suspicious code where multiple if have the same expression (e.g "if (a) { } else if (a) { }") */
|
||||||
void checkDuplicateIf();
|
void checkDuplicateIf();
|
||||||
|
|
||||||
|
/** @brief %Check for suspicious code where if and else branch are the same (e.g "if (a) b = true; else b = true;") */
|
||||||
|
void checkDuplicateBranch();
|
||||||
|
|
||||||
// Error messages..
|
// Error messages..
|
||||||
void cstyleCastError(const Token *tok);
|
void cstyleCastError(const Token *tok);
|
||||||
void dangerousUsageStrtolError(const Token *tok);
|
void dangerousUsageStrtolError(const Token *tok);
|
||||||
|
@ -237,6 +241,7 @@ public:
|
||||||
void incrementBooleanError(const Token *tok);
|
void incrementBooleanError(const Token *tok);
|
||||||
void comparisonOfBoolWithIntError(const Token *tok, const std::string &varname);
|
void comparisonOfBoolWithIntError(const Token *tok, const std::string &varname);
|
||||||
void duplicateIfError(const Token *tok1, const Token *tok2);
|
void duplicateIfError(const Token *tok1, const Token *tok2);
|
||||||
|
void duplicateBranchError(const Token *tok1, const Token *tok2);
|
||||||
|
|
||||||
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings)
|
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings)
|
||||||
{
|
{
|
||||||
|
@ -282,6 +287,7 @@ public:
|
||||||
c.incrementBooleanError(0);
|
c.incrementBooleanError(0);
|
||||||
c.comparisonOfBoolWithIntError(0, "varname");
|
c.comparisonOfBoolWithIntError(0, "varname");
|
||||||
c.duplicateIfError(0, 0);
|
c.duplicateIfError(0, 0);
|
||||||
|
c.duplicateBranchError(0, 0);
|
||||||
}
|
}
|
||||||
|
|
||||||
std::string myName() const
|
std::string myName() const
|
||||||
|
|
|
@ -114,6 +114,7 @@ private:
|
||||||
TEST_CASE(comparisonOfBoolWithInt);
|
TEST_CASE(comparisonOfBoolWithInt);
|
||||||
|
|
||||||
TEST_CASE(duplicateIf);
|
TEST_CASE(duplicateIf);
|
||||||
|
TEST_CASE(duplicateBranch);
|
||||||
}
|
}
|
||||||
|
|
||||||
void check(const char code[], const char *filename = NULL)
|
void check(const char code[], const char *filename = NULL)
|
||||||
|
@ -138,6 +139,7 @@ private:
|
||||||
checkOther.checkSizeofForArrayParameter();
|
checkOther.checkSizeofForArrayParameter();
|
||||||
checkOther.clarifyCondition();
|
checkOther.clarifyCondition();
|
||||||
checkOther.checkDuplicateIf();
|
checkOther.checkDuplicateIf();
|
||||||
|
checkOther.checkDuplicateBranch();
|
||||||
|
|
||||||
// Simplify token list..
|
// Simplify token list..
|
||||||
tokenizer.simplifyTokenList();
|
tokenizer.simplifyTokenList();
|
||||||
|
@ -2453,6 +2455,28 @@ private:
|
||||||
"}");
|
"}");
|
||||||
ASSERT_EQUALS("", errout.str());
|
ASSERT_EQUALS("", errout.str());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
void duplicateBranch()
|
||||||
|
{
|
||||||
|
check("void f(int a, int &b) {\n"
|
||||||
|
" if (a)\n"
|
||||||
|
" b = 1;\n"
|
||||||
|
" else\n"
|
||||||
|
" b = 1;\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2]: (style) Found duplicate branches for if and else.\n", errout.str());
|
||||||
|
|
||||||
|
check("void f(int a, int &b) {\n"
|
||||||
|
" if (a) {\n"
|
||||||
|
" if (a == 1)\n"
|
||||||
|
" b = 2;\n"
|
||||||
|
" else\n"
|
||||||
|
" b = 2;\n"
|
||||||
|
" } else\n"
|
||||||
|
" b = 1;\n"
|
||||||
|
"}");
|
||||||
|
ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (style) Found duplicate branches for if and else.\n", errout.str());
|
||||||
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
REGISTER_TEST(TestOther)
|
REGISTER_TEST(TestOther)
|
||||||
|
|
Loading…
Reference in New Issue