Refactoring checks of overlapping conditions in if and else-if

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<std::string> &constFunctions);
 
+static bool isOverlappingCond(const Token * const cond1, const Token * const cond2, const std::set<std::string> &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<Scope>::const_iterator i = symbolDatabase->scopeList.begin(); i != symbolDatabase->scopeList.end(); ++i) {
-        if (i->type == Scope::eIf && Token::Match(i->classDef, "if ( %var% & %num% ) {")) {
+        if (i->type != Scope::eIf || !Token::simpleMatch(i->classDef, "if ("))
-            const Token* const tok = i->classDef;
+            continue;
-            const unsigned int varid(tok->tokAt(2)->varId());
-            if (varid == 0)
-                continue;
 
-            const MathLib::bigint num1 = MathLib::toLongNumber(tok->strAt(4));
+        const Token * const cond1 = i->classDef->next()->astOperand2();
-            if (num1 < 0)
-                continue;
 
-            const Token *tok2 = tok->linkAt(6);
+        const Token * tok2 = i->classDef->next();
-            while (Token::simpleMatch(tok2, "} else { if (")) {
+        while (tok2) {
-                // Goto '('
+            tok2 = tok2->link();
-                const Token * const opar = tok2->tokAt(4);
+            if (!Token::simpleMatch(tok2, ") {"))
+                break;
+            tok2 = tok2->linkAt(1);
+            if (!Token::simpleMatch(tok2, "} else { if ("))
+                break;
+            tok2 = tok2->tokAt(4);
 
-                // tok2: skip if-block
+            if (isOverlappingCond(cond1, tok2->astOperand2(), _settings->library.functionpure))
-                tok2 = opar->link();
+                multiConditionError(tok2, cond1->linenr());
-                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());
-                    }
-                }
-            }
         }
     }
 }

lib/checkother.cpp

@@ -72,7 +72,7 @@ static bool isConstExpression(const Token *tok, const std::set<std::string> &con
     return isConstExpression(tok->astOperand1(),constFunctions) && isConstExpression(tok->astOperand2(),constFunctions);
 }
 
-static bool isSameExpression(const Token *tok1, const Token *tok2, const std::set<std::string> &constFunctions)
+bool isSameExpression(const Token *tok1, const Token *tok2, const std::set<std::string> &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<Scope>::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<std::string, const Token*> 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<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(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<const Token *> 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; }

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<std::string> &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");

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)

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"