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

Merged checkSelfAssignment() into checkDuplicateExpression(): 

- Fixed false negatives on self assignments of more complex expressions like "a.b"
- New false negative on self assignment in initialization
- Support this->... pattern in isSameExpression()
- Fixed #5819: Check type of operands
This commit is contained in:
PKEuS 2014-05-21 18:19:42 +02:00
parent 7b1eca700b
commit cdfed32500
3 changed files with 76 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

104
lib/checkother.cpp
View File

@ -78,6 +78,10 @@ bool isSameExpression(const Token *tok1, const Token *tok2, const std::set<std::
return true;
if (tok1 == nullptr || tok2 == nullptr)
return false;
if (tok1->str() == "." && tok1->astOperand1() && tok1->astOperand1()->str() == "this")
tok1 = tok1->astOperand2();
if (tok2->str() == "." && tok2->astOperand1() && tok2->astOperand1()->str() == "this")
tok2 = tok2->astOperand2();
if (tok1->str() != tok2->str() || tok1->varId() != tok2->varId())
return false;
if (tok1->str() == "." && tok1->originalName() != tok2->originalName())
@ -1152,63 +1156,6 @@ void CheckOther::suspiciousEqualityComparisonError(const Token* tok)
}
//---------------------------------------------------------------------------
// int x = 1;
// x = x; // <- redundant assignment to self
//
// int y = y; // <- redundant initialization to self
//---------------------------------------------------------------------------
static bool isTypeWithoutSideEffects(const Tokenizer *tokenizer, const Variable* var)
{
return ((var && (!var->isClass() || var->isPointer() || var->isStlType())) || !tokenizer->isCPP());
}
static inline const Token *findSelfAssignPattern(const Token *start)
{
return Token::findmatch(start, "%var% = %var% ;|=|)");
}
void CheckOther::checkSelfAssignment()
{
if (!_settings->isEnabled("warning"))
return;
const Token *tok = findSelfAssignPattern(_tokenizer->tokens());
while (tok) {
if (Token::Match(tok->previous(), "[;{}.]") &&
tok->varId() && tok->varId() == tok->tokAt(2)->varId() &&
isTypeWithoutSideEffects(_tokenizer, tok->variable())) {
bool err = true;
// no false positive for 'x = x ? x : 1;'
// it is simplified to 'if (x) { x=x; } else { x=1; }'. The simplification
// always write all tokens on 1 line (even if the statement is several lines), so
// check if the linenr is the same for all the tokens.
if (Token::Match(tok->tokAt(-2), ") { %var% = %var% ; } else { %varid% =", tok->varId())) {
// Find the 'if' token
const Token *tokif = tok->linkAt(-2)->previous();
// find the '}' that terminates the 'else'-block
const Token *else_end = tok->linkAt(6);
if (tokif && else_end && tokif->linenr() == else_end->linenr())
err = false;
}
if (err)
selfAssignmentError(tok, tok->str());
}
tok = findSelfAssignPattern(tok->next());
}
}
void CheckOther::selfAssignmentError(const Token *tok, const std::string &varname)
{
reportError(tok, Severity::warning,
"selfAssignment", "Redundant assignment of '" + varname + "' to itself.");
}
//---------------------------------------------------------------------------
// if ((x != 1) || (x != 3)) // expression always true
// if ((x == 1) && (x == 3)) // expression always false
@ -2778,6 +2725,21 @@ namespace {
// (x == x), (x && x), (x || x)
// (x.y == x.y), (x.y && x.y), (x.y || x.y)
//---------------------------------------------------------------------------
static bool isWithoutSideEffects(const Tokenizer *tokenizer, const Token* tok)
{
if (!tokenizer->isCPP())
return true;
while (tok && tok->astOperand2() && tok->astOperand2()->str() != "(")
tok = tok->astOperand2();
if (tok->varId()) {
const Variable* var = tok->variable();
return var && (!var->isClass() || var->isPointer() || var->isStlType());
}
return true;
}
void CheckOther::checkDuplicateExpression()
{
if (!_settings->isEnabled("style"))
@ -2796,22 +2758,30 @@ void CheckOther::checkDuplicateExpression()
continue;
for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) {
if (tok->isOp() && tok->astOperand1() && !Token::Match(tok, "+|*|=|<<|>>")) {
if (tok->isOp() && tok->astOperand1() && !Token::Match(tok, "+|*|<<|>>")) {
bool assignment = tok->str() == "=";
if (Token::Match(tok, "==|!=|-") && astIsFloat(tok->astOperand1(), true))
continue;
if (isSameExpression(tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure))
duplicateExpressionError(tok, tok, tok->str());
else if (!Token::Match(tok, "[-/%]")) { // These operators are not associative
if (tok->astOperand2() && tok->str() == tok->astOperand1()->str() && isSameExpression(tok->astOperand2(), tok->astOperand1()->astOperand2(), _settings->library.functionpure))
if (isSameExpression(tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) {
if (isWithoutSideEffects(_tokenizer, tok->astOperand1())) {
if (assignment) {
// Need a hack to cope with our ternary operator simplification:
if (!Token::Match(tok->tokAt(-2), "{ %var% = %var% ; } else { %varid% = !!%varid%", tok->previous()->varId()))
selfAssignmentError(tok, tok->strAt(-1));
} else
duplicateExpressionError(tok, tok, tok->str());
}
} else if (!Token::Match(tok, "[-/%]")) { // These operators are not associative
if (tok->astOperand2() && tok->str() == tok->astOperand1()->str() && isSameExpression(tok->astOperand2(), tok->astOperand1()->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer, tok->astOperand2()))
duplicateExpressionError(tok->astOperand2(), tok->astOperand2(), tok->str());
else if (tok->astOperand2()) {
const Token *ast1 = tok->astOperand1();
while (ast1 && tok->str() == ast1->str()) {
if (isSameExpression(ast1->astOperand1(), tok->astOperand2(), _settings->library.functionpure))
if (isSameExpression(ast1->astOperand1(), tok->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer, ast1->astOperand1()))
// TODO: warn if variables are unchanged. See #5683
// Probably the message should be changed to 'duplicate expressions X in condition or something like that'.
;//duplicateExpressionError(ast1->astOperand1(), tok->astOperand2(), tok->str());
else if (isSameExpression(ast1->astOperand2(), tok->astOperand2(), _settings->library.functionpure))
else if (isSameExpression(ast1->astOperand2(), tok->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer, ast1->astOperand2()))
duplicateExpressionError(ast1->astOperand2(), tok->astOperand2(), tok->str());
if (!isConstExpression(ast1->astOperand2(), _settings->library.functionpure))
break;
@ -2836,6 +2806,12 @@ void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2,
"determine if it is correct.");
}
void CheckOther::selfAssignmentError(const Token *tok, const std::string &varname)
{
reportError(tok, Severity::warning,
"selfAssignment", "Redundant assignment of '" + varname + "' to itself.");
}
//---------------------------------------------------------------------------
// Check for string comparison involving two static strings.
// if(strcmp("00FF00","00FF00")==0) // <- statement is always true

4
lib/checkother.h
View File

@ -64,7 +64,6 @@ public:
checkOther.checkRedundantAssignment();
checkOther.checkRedundantAssignmentInSwitch();
checkOther.checkSuspiciousCaseInSwitch();
checkOther.checkSelfAssignment();
checkOther.checkDuplicateBranch();
checkOther.checkDuplicateExpression();
checkOther.checkUnreachableCode();
@ -194,9 +193,6 @@ public:
/** @brief %Check for switch case fall through without comment */
void checkSwitchCaseFallThrough();
/** @brief %Check for assigning a variable to itself*/
void checkSelfAssignment();
/** @brief %Check for testing for mutual exclusion over ||*/
void checkIncorrectLogicOperator();

46
test/testother.cpp
View File

@ -3384,23 +3384,29 @@ private:
void selfAssignment() {
check("void foo()\n"
"{\n"
" int x = 1;\n"
" x = x;\n"
" return 0;\n"
" int x = 1;\n"
" x = x;\n"
" return 0;\n"
"}");
ASSERT_EQUALS("[test.cpp:4]: (warning) Redundant assignment of 'x' to itself.\n", errout.str());
check("void foo()\n"
"{\n"
" int x = x;\n"
" int x = x;\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'x' to itself.\n", errout.str());
check("void foo()\n"
"{\n"
" std::string var = var = \"test\";\n"
" std::string var = var = \"test\";\n"
"}");
ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'var' to itself.\n", errout.str());
TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'var' to itself.\n", "", errout.str());
check("struct A { int b; };\n"
"void foo(A* a1, A* a2) {\n"
" a1->b = a1->b;\n"
"}");
TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'a1->b' to itself.\n", "[test.cpp:3]: (warning) Redundant assignment of 'b' to itself.\n", errout.str());
// #4073 (segmentation fault)
check("void Foo::myFunc( int a )\n"
@ -3411,9 +3417,9 @@ private:
check("void foo()\n"
"{\n"
" int x = 1;\n"
" x = x + 1;\n"
" return 0;\n"
" int x = 1;\n"
" x = x + 1;\n"
" return 0;\n"
"}");
ASSERT_EQUALS("", errout.str());
@ -3433,7 +3439,7 @@ private:
// #2502 - non-primitive type -> there might be some side effects
check("void foo()\n"
"{\n"
" Fred fred; fred = fred;\n"
" Fred fred; fred = fred;\n"
"}");
ASSERT_EQUALS("", errout.str());
@ -3471,6 +3477,15 @@ private:
" this->var = var;\n"
"}");
ASSERT_EQUALS("[test.cpp:6]: (warning) Redundant assignment of 'var' to itself.\n", errout.str());
check("class Foo {\n"
" int var;\n"
" void func(int var);\n"
"};\n"
"void Foo::func(int var) {\n"
" this->var = var;\n"
"}");
ASSERT_EQUALS("", errout.str());
}
void trac1132() {
@ -4998,6 +5013,17 @@ private:
"}\n", "test.cpp", false, false, false, false); // don't run simplifications
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str());
// #5819
check("Vector func(Vector vec1) {\n"
" return fabs(vec1 & vec1 & vec1);\n"
"}");
ASSERT_EQUALS("", errout.str());
check("Vector func(int vec1) {\n"
" return fabs(vec1 & vec1 & vec1);\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&'.\n", errout.str());
}
void duplicateExpression4() {