diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 88bd6bf29..84add5554 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -78,6 +78,10 @@ bool isSameExpression(const Token *tok1, const Token *tok2, const std::setstr() == "." && 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 diff --git a/lib/checkother.h b/lib/checkother.h index 32c74a2bb..ed96853bb 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -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(); diff --git a/test/testother.cpp b/test/testother.cpp index f19985d75..5d6dab69e 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -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() {