Fixed oppositeInnerCondition check:

- Resolved false positives #4170 and #4186, as well as numerous other potential false positives
- Improved message to point to both locations
- Inner condition could also be a while loop; Outer if could also be 'else if'
- Made the check non-experimental again (#3645)

lib/checkother.cpp

@@ -3377,59 +3377,56 @@ void CheckOther::incompleteArrayFillError(const Token* tok, const std::string& b
 
 void CheckOther::oppositeInnerCondition()
 {
-    // FIXME: This check is experimental because of #4170 and #4186. Fix those tickets and remove the "experimental".
+    if (!_settings->isEnabled("warning") || !_settings->inconclusive)
-    if (!_settings->isEnabled("warning") || !_settings->inconclusive || !_settings->experimental)
         return;
 
     const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
 
     for (std::list<Scope>::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
-        const Token* const toke = scope->classDef;
+        if (scope->type != Scope::eIf && scope->type != Scope::eElseIf)
+            continue;
 
+        const Token *op1Tok, *op2Tok;
+        op1Tok = scope->classDef->tokAt(2);
+        op2Tok = scope->classDef->tokAt(4);
 
-        if (scope->type == Scope::eIf && toke) {
+        if (scope->classDef->strAt(6) == "{") {
 
-            const Token *op1Tok, *op2Tok;
+            const char *oppositeCondition = nullptr;
-            op1Tok = scope->classDef->tokAt(2);
-            op2Tok = scope->classDef->tokAt(4);
 
-            if (scope->classDef->strAt(6) == "{") {
+            if (scope->classDef->strAt(3) == "==")
+                oppositeCondition = "if|while ( %any% !=|<|>|<=|>= %any% )";
+            else if (scope->classDef->strAt(3) == "!=")
+                oppositeCondition = "if|while ( %any% ==|>=|<= %any% )";
+            else if (scope->classDef->strAt(3) == "<")
+                oppositeCondition = "if|while ( %any% >|>=|== %any% )";
+            else if (scope->classDef->strAt(3) == "<=")
+                oppositeCondition = "if|while ( %any% > %any% )";
+            else if (scope->classDef->strAt(3) == ">")
+                oppositeCondition = "if|while ( %any% <|<=|== %any% )";
+            else if (scope->classDef->strAt(3) == ">=")
+                oppositeCondition = "if|while ( %any% < %any% )";
 
-                const char *oppositeCondition = nullptr;
+            if (oppositeCondition) {
+                int flag = 0;
 
-                if (scope->classDef->strAt(3) == "==")
+                for (const Token* tok = scope->classStart; tok != scope->classEnd && flag == 0; tok = tok->next()) {
-                    oppositeCondition = "if ( %any% !=|<|>|<=|>= %any% )";
+                    if ((tok->str() == op1Tok->str() || tok->str() == op2Tok->str()) && (tok->next()->isAssignmentOp() || tok->next()->type() == Token::eIncDecOp))
-                else if (scope->classDef->strAt(3) == "!=")
+                        break;
-                    oppositeCondition = "if ( %any% ==|>=|<= %any% )";
-                else if (scope->classDef->strAt(3) == "<")
-                    oppositeCondition = "if ( %any% >|>=|== %any% )";
-                else if (scope->classDef->strAt(3) == "<=")
-                    oppositeCondition = "if ( %any% > %any% )";
-                else if (scope->classDef->strAt(3) == ">")
-                    oppositeCondition = "if ( %any% <|<=|== %any% )";
-                else if (scope->classDef->strAt(3) == ">=")
-                    oppositeCondition = "if ( %any% < %any% )";
 
-                if (oppositeCondition) {
+                    if (Token::Match(tok, oppositeCondition)) {
-                    int flag = 0;
+                        if ((tok->strAt(2) == op1Tok->str() && tok->strAt(4) == op2Tok->str()) || (tok->strAt(2) == op2Tok->str() && tok->strAt(4) == op1Tok->str()))
-
+                            oppositeInnerConditionError(scope->classDef, tok);
-                    for (const Token* tok = scope->classStart; tok != scope->classEnd && flag == 0; tok = tok->next()) {
+                    } else if (Token::Match(tok, "%any% (")) {
-                        if ((tok->str() == op1Tok->str() || tok->str() == op2Tok->str()) && tok->strAt(1) == "=")
+                        if (tok->function()) { // Check if it is a member function. If yes: bailout (TODO: this causes false negatives, since we do not check of which function this is a member
-                            break;
+                            const Function* func = tok->function();
-                        else if (Token::Match(tok, "%any% ( %any% )")) {
+                            if (!func->isConst && (func->access == Private || func->access == Protected || func->access == Public))
-                            if ((tok->strAt(2) == op1Tok->str() || tok->strAt(2) == op2Tok->str()))
                                 break;
-                        } else if (Token::Match(tok, "%any% ( %any% , %any%")) {
-                            for (const Token* tok2 = tok->next(); tok2 != tok->linkAt(1); tok2 = tok2->next()) {
-                                if (tok2->str() == op1Tok->str()) {
-                                    flag = 1;
-                                    break;
-                                }
-                            }
-                        } else if (Token::Match(tok, oppositeCondition)) {
-                            if ((tok->strAt(2) == op1Tok->str() && tok->strAt(4) == op2Tok->str()) || (tok->strAt(2) == op2Tok->str() && tok->strAt(4) == op1Tok->str()))
-                                oppositeInnerConditionError(toke);
                         }
+                        if (op1Tok->varId() && Token::findmatch(tok->next(), "%varid%", tok->linkAt(1), op1Tok->varId()) != nullptr)
+                            break;
+                        if (op2Tok->varId() && Token::findmatch(tok->next(), "%varid%", tok->linkAt(1), op2Tok->varId()) != nullptr)
+                            break;
                     }
                 }
             }
@@ -3437,9 +3434,12 @@ void CheckOther::oppositeInnerCondition()
     }
 }
 
-void CheckOther::oppositeInnerConditionError(const Token *tok)
+void CheckOther::oppositeInnerConditionError(const Token *tok1, const Token* tok2)
 {
-    reportError(tok, Severity::warning, "oppositeInnerCondition", "Opposite conditions in nested 'if' blocks lead to a dead code block.", true);
+    std::list<const Token*> callstack;
+    callstack.push_back(tok1);
+    callstack.push_back(tok2);
+    reportError(callstack, Severity::warning, "oppositeInnerCondition", "Opposite conditions in nested 'if' blocks lead to a dead code block.", true);
 }
 
 

lib/checkother.h

@@ -273,7 +273,7 @@ private:
     void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result);
     void checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName);
     void checkPipeParameterSizeError(const Token *tok, const std::string &strVarName, const std::string &strDim);
-    void oppositeInnerConditionError(const Token *tok);
+    void oppositeInnerConditionError(const Token *tok1, const Token* tok2);
     void clarifyCalculationError(const Token *tok, const std::string &op);
     void clarifyConditionError(const Token *tok, bool assign, bool boolop);
     void clarifyStatementError(const Token* tok);
@@ -357,7 +357,7 @@ private:
         // style/warning
         c.checkComparisonFunctionIsAlwaysTrueOrFalseError(0,"isless","varName",false);
         c.checkCastIntToCharAndBackError(0,"func_name");
-        c.oppositeInnerConditionError(0);
+        c.oppositeInnerConditionError(0, 0);
         c.cstyleCastError(0);
         c.passedByValueError(0, "parametername");
         c.constStatementError(0, "type");

test/testother.cpp

@@ -297,13 +297,52 @@ private:
 
 
     void oppositeInnerCondition() {
-        check("void foo(int a, int b)\n"
+        check("void foo(int a, int b) {\n"
-              "{\n"
               "    if(a==b)\n"
               "        if(a!=b)\n"
               "            cout << a;\n"
-              "}", "test.cpp", true, true);
+              "}");
-        ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str());
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning, inconclusive) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str());
+
+        check("void foo(int a) {\n"
+              "    if(a >= 50) {\n"
+              "        if(a < 50)\n"
+              "            cout << a;\n"
+              "        else\n"
+              "            cout << 100;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (warning, inconclusive) Opposite conditions in nested 'if' blocks lead to a dead code block.\n", errout.str());
+
+        // #4186
+        check("void foo(int a) {\n"
+              "    if(a >= 50) {\n"
+              "        if(a > 50)\n"
+              "            cout << a;\n"
+              "        else\n"
+              "            cout << 100;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        // 4170
+        check("class foo {\n"
+              "    void bar() {\n"
+              "        if (tok == '(') {\n"
+              "            next();\n"
+              "            if (tok == ',') {\n"
+              "                next();\n"
+              "                if (tok != ',') {\n"
+              "                    op->reg2 = asm_parse_reg();\n"
+              "                }\n"
+              "                skip(',');\n"
+              "            }\n"
+              "        }\n"
+              "    }\n"
+              "    void next();\n"
+              "    const char *tok;\n"
+              "};");
+        ASSERT_EQUALS("", errout.str());
 
         check("void foo(int i)\n"
               "{\n"
@@ -313,7 +352,7 @@ private:
               "           cout << a;\n"
               "       }\n"
               "    }\n"
-              "}", "test.cpp", true, true);
+              "}");
         ASSERT_EQUALS("", errout.str());
 
 
@@ -328,7 +367,7 @@ private:
               "        if(i<5){\n"
               "        }\n"
               "    }\n"
-              "}", "test.cpp", true, true);
+              "}");
         ASSERT_EQUALS("", errout.str());
     }