diff --git a/lib/checkassignif.cpp b/lib/checkassignif.cpp index 88be4698b..c5c20ec74 100644 --- a/lib/checkassignif.cpp +++ b/lib/checkassignif.cpp @@ -249,9 +249,44 @@ void CheckAssignIf::comparisonError(const Token *tok, const std::string &bitop, reportError(tok, Severity::style, "comparisonError", errmsg); } +extern bool isSameExpression(const Token *tok1, const Token *tok2, const std::set &constFunctions); +static bool isOverlappingCond(const Token * const cond1, const Token * const cond2, const std::set &constFunctions) +{ + if (!cond1 || !cond2) + return false; + // same expressions + if (isSameExpression(cond1,cond2,constFunctions)) + return true; + // bitwise overlap for example 'x&7' and 'x==1' + if (cond1->str() == "&" && cond1->astOperand1() && cond2->astOperand2()) { + const Token *expr1 = cond1->astOperand1(); + const Token *num1 = cond1->astOperand2(); + if (!num1->isNumber()) + std::swap(expr1,num1); + if (!num1->isNumber() || MathLib::isNegative(num1->str())) + return false; + + if (!Token::Match(cond2, "&|==") || !cond2->astOperand1() || !cond2->astOperand2()) + return false; + const Token *expr2 = cond2->astOperand1(); + const Token *num2 = cond2->astOperand2(); + if (!num2->isNumber()) + std::swap(expr2,num2); + if (!num2->isNumber() || MathLib::isNegative(num2->str())) + return false; + + if (!isSameExpression(expr1,expr2,constFunctions)) + return false; + + const MathLib::bigint value1 = MathLib::toLongNumber(num1->str()); + const MathLib::bigint value2 = MathLib::toLongNumber(num2->str()); + return ((value1 & value2) == value2); + } + return false; +} void CheckAssignIf::multiCondition() @@ -262,37 +297,23 @@ void CheckAssignIf::multiCondition() const SymbolDatabase* const symbolDatabase = _tokenizer->getSymbolDatabase(); for (std::list::const_iterator i = symbolDatabase->scopeList.begin(); i != symbolDatabase->scopeList.end(); ++i) { - if (i->type == Scope::eIf && Token::Match(i->classDef, "if ( %var% & %num% ) {")) { - const Token* const tok = i->classDef; - const unsigned int varid(tok->tokAt(2)->varId()); - if (varid == 0) - continue; + if (i->type != Scope::eIf || !Token::simpleMatch(i->classDef, "if (")) + continue; - const MathLib::bigint num1 = MathLib::toLongNumber(tok->strAt(4)); - if (num1 < 0) - continue; + const Token * const cond1 = i->classDef->next()->astOperand2(); - const Token *tok2 = tok->linkAt(6); - while (Token::simpleMatch(tok2, "} else { if (")) { - // Goto '(' - const Token * const opar = tok2->tokAt(4); + const Token * tok2 = i->classDef->next(); + while (tok2) { + tok2 = tok2->link(); + if (!Token::simpleMatch(tok2, ") {")) + break; + tok2 = tok2->linkAt(1); + if (!Token::simpleMatch(tok2, "} else { if (")) + break; + tok2 = tok2->tokAt(4); - // tok2: skip if-block - tok2 = opar->link(); - if (Token::simpleMatch(tok2, ") {")) - tok2 = tok2->next()->link(); - - // check condition.. - if (Token::Match(opar, "( %varid% ==|& %num% &&|%oror%|)", varid)) { - const MathLib::bigint num2 = MathLib::toLongNumber(opar->strAt(3)); - if (num2 < 0) - continue; - - if ((num1 & num2) == num2) { - multiConditionError(opar, tok->linenr()); - } - } - } + if (isOverlappingCond(cond1, tok2->astOperand2(), _settings->library.functionpure)) + multiConditionError(tok2, cond1->linenr()); } } } diff --git a/lib/checkother.cpp b/lib/checkother.cpp index acb97d039..10ea2d913 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -72,7 +72,7 @@ static bool isConstExpression(const Token *tok, const std::set &con return isConstExpression(tok->astOperand1(),constFunctions) && isConstExpression(tok->astOperand2(),constFunctions); } -static bool isSameExpression(const Token *tok1, const Token *tok2, const std::set &constFunctions) +bool isSameExpression(const Token *tok1, const Token *tok2, const std::set &constFunctions) { if (tok1 == nullptr && tok2 == nullptr) return true; @@ -166,6 +166,7 @@ static bool isOppositeCond(const Token * const cond1, const Token * const cond2, (comp1 == ">=" && comp2 == "<=")); } + //---------------------------------------------------------------------------------- // The return value of fgetc(), getc(), ungetc(), getchar() etc. is an integer value. // If this return value is stored in a character variable and then compared @@ -2559,69 +2560,6 @@ static bool expressionHasSideEffects(const Token *first, const Token *last) return false; } -void CheckOther::checkDuplicateIf() -{ - if (!_settings->isEnabled("style")) - return; - - const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); - - for (std::list::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - const Token* const tok = scope->classDef; - // only check if statements - if (scope->type != Scope::eIf || !tok) - continue; - - std::map expressionMap; - - // get the expression from the token stream - std::string expression = tok->tokAt(2)->stringifyList(tok->next()->link()); - - // save the expression and its location - expressionMap[expression] = tok; - - // find the next else if (...) statement - const Token *tok1 = scope->classEnd; - - // check all the else if (...) statements - while (Token::simpleMatch(tok1, "} else { if (") && - Token::simpleMatch(tok1->linkAt(4), ") {")) { - int conditionIndex=(tok1->strAt(3)=="(") ? 3 : 4; - // get the expression from the token stream - expression = tok1->tokAt(conditionIndex+1)->stringifyList(tok1->linkAt(conditionIndex)); - - // try to look up the expression to check for duplicates - std::map::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(conditionIndex+1), tok1->linkAt(conditionIndex)->previous())) - duplicateIfError(it->second, tok1->next()); - } - - // not a duplicate expression so save it and its location - else - expressionMap[expression] = tok1->next(); - - // find the next else if (...) statement - tok1 = tok1->linkAt(conditionIndex)->next()->link(); - } - } -} - -void CheckOther::duplicateIfError(const Token *tok1, const Token *tok2) -{ - std::list toks; - toks.push_back(tok2); - toks.push_back(tok1); - - reportError(toks, Severity::style, "duplicateIf", "Duplicate conditions in 'if' and related 'else if'.\n" - "Duplicate conditions in 'if' and related 'else if'. This is suspicious and might indicate " - "a cut and paste or logic error. Please examine this code carefully to determine " - "if it is correct."); -} - //----------------------------------------------------------------------------- // check for duplicate code in if and else branches // if (a) { b = true; } else { b = true; } diff --git a/lib/checkother.h b/lib/checkother.h index 5909857b9..0b1248a28 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -30,6 +30,10 @@ class Token; class Function; class Variable; +/** Is expressions same? */ +bool isSameExpression(const Token *tok1, const Token *tok2, const std::set &constFunctions); + + /// @addtogroup Checks /// @{ @@ -61,7 +65,6 @@ public: checkOther.checkRedundantAssignmentInSwitch(); checkOther.checkSuspiciousCaseInSwitch(); checkOther.checkSelfAssignment(); - checkOther.checkDuplicateIf(); checkOther.checkDuplicateBranch(); checkOther.checkDuplicateExpression(); checkOther.checkUnreachableCode(); @@ -382,7 +385,6 @@ private: c.incorrectStringCompareError(0, "substr", "\"Hello World\""); c.suspiciousStringCompareError(0, "foo"); c.incorrectStringBooleanError(0, "\"Hello World\""); - c.duplicateIfError(0, 0); c.duplicateBranchError(0, 0); c.duplicateExpressionError(0, 0, "&&"); c.alwaysTrueFalseStringCompareError(0, "str1", "str2"); diff --git a/test/testassignif.cpp b/test/testassignif.cpp index 262ec2a86..f98ba22e4 100644 --- a/test/testassignif.cpp +++ b/test/testassignif.cpp @@ -37,6 +37,7 @@ private: TEST_CASE(mismatchingBitAnd); // overlapping bitmasks TEST_CASE(compare); // mismatching LHS/RHS in comparison TEST_CASE(multicompare); // mismatching comparisons + TEST_CASE(duplicateIf); // duplicate conditions in if and else-if } void check(const char code[]) { @@ -283,6 +284,82 @@ private: "}"); ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str()); } + + void duplicateIf() { + check("void f(int a, int &b) {\n" + " if (a) { b = 1; }\n" + " else { if (a) { b = 2; } }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str()); + + check("void f(int a, int &b) {\n" + " if (a) { b = 1; }\n" + " else { if (a) { b = 2; } }\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\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 == 1) { b = 3; } } }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\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]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\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" + " x = x / 2;\n" + " if (x < 100) { b = 1; }\n" + " else { x = x / 2; if (x < 100) { b = 2; } }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int i) {\n" + " if(i == 0x02e2000000 || i == 0xa0c6000000)\n" + " foo(i);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // ticket 3689 ( avoid false positive ) + check("int fitInt(long long int nValue){\n" + " if( nValue < 0x7fffffffLL )\n" + " {\n" + " return 32;\n" + " }\n" + " if( nValue < 0x7fffffffffffLL )\n" + " {\n" + " return 48;\n" + " }\n" + " else {\n" + " if( nValue < 0x7fffffffffffffffLL )\n" + " {\n" + " return 64;\n" + " } else\n" + " {\n" + " return -1;\n" + " }\n" + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestAssignIf) diff --git a/test/testother.cpp b/test/testother.cpp index 55c5209fd..f5699c0e5 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -153,8 +153,6 @@ private: TEST_CASE(incorrectStringCompare); - TEST_CASE(duplicateIf); - TEST_CASE(duplicateIf1); // ticket 3689 TEST_CASE(duplicateBranch); TEST_CASE(duplicateBranch1); // tests extracted by http://www.viva64.com/en/b/0149/ ( Comparison between PVS-Studio and cppcheck ): Errors detected in Quake 3: Arena by PVS-Studio: Fragement 2 TEST_CASE(duplicateBranch2); // empty macro @@ -4502,91 +4500,6 @@ private: ASSERT_EQUALS("", errout.str()); } - - void duplicateIf() { - check("void f(int a, int &b) {\n" - " if (a) { b = 1; }\n" - " else { if (a) { b = 2; } }\n" - "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Duplicate conditions in 'if' and related 'else if'.\n", errout.str()); - - check("void f(int a, int &b) {\n" - " if (a) { b = 1; }\n" - " else { if (a) { b = 2; } }\n" - "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Duplicate conditions in 'if' and related 'else if'.\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 == 1) { b = 3; } } }\n" - "}"); - ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2]: (style) Duplicate conditions in 'if' and related 'else if'.\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) Duplicate conditions in 'if' and related 'else if'.\n", errout.str()); - - check("void f(int a, int &b) {\n" - " if (a == 1) {\n" - " b = 1;\n" - " if (b == 1) { }\n" // condition is always true. must skip simplifications - " else if (b == 1) { }\n" - " } else if (a == 2) { b = 2; }\n" - " else if (a == 2) { b = 3; }\n" - "}", 0, false, false, false, false); - ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (style) Duplicate conditions in 'if' and related 'else if'.\n" - "[test.cpp:5] -> [test.cpp:4]: (style) Duplicate conditions in 'if' and related 'else if'.\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" - " x = x / 2;\n" - " if (x < 100) { b = 1; }\n" - " else { x = x / 2; if (x < 100) { b = 2; } }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(int i) {\n" - " if(i == 0x02e2000000 || i == 0xa0c6000000)\n" - " foo(i);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f(const std::string &token)\n" - "{\n" - " if( token == \"C\")\n" - " {\n" - " std::cout << \"C\";\n" - " }\n" - " else { if ( token == \"A\" )\n" - " {\n" - " std::cout << \"A\";\n" - " }\n" - " else { if ( token == \"A\" )\n" - " {\n" - " std::cout << \"A\";\n" - " }\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("[test.cpp:11] -> [test.cpp:7]: (style) Duplicate conditions in 'if' and related 'else if'.\n", errout.str()); - } - void duplicateBranch() { check("void f(int a, int &b) {\n" " if (a)\n" @@ -4818,30 +4731,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void duplicateIf1() { // ticket 3689 ( avoid false positive ) - - check("int fitInt(long long int nValue){\n" - " if( nValue < 0x7fffffffLL )\n" - " {\n" - " return 32;\n" - " }\n" - " if( nValue < 0x7fffffffffffLL )\n" - " {\n" - " return 48;\n" - " }\n" - " else {\n" - " if( nValue < 0x7fffffffffffffffLL )\n" - " {\n" - " return 64;\n" - " } else\n" - " {\n" - " return -1;\n" - " }\n" - " }\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } - void duplicateExpression2() { // check if float is NaN or Inf check("int f(long double ldbl, double dbl, float flt) {\n" // ticket #2730 " if (ldbl != ldbl) have_nan = 1;\n"