Fixed #2162 (false positive: Mutual exclusion over ||)

cli/cmdlineparser.cpp

@@ -293,8 +293,8 @@ bool CmdLineParser::ParseFromArgs(int argc, const char* const argv[])
 
             if (_settings->_jobs > 10000)
             {
-                // This limit is here just to catch typos. If someone has 
+                // This limit is here just to catch typos. If someone has
-                // need for more jobs, this value should be increased. 
+                // need for more jobs, this value should be increased.
                 PrintMessage("cppcheck: argument for '-j' is allowed to be 10000 at max");
                 return false;
             }

lib/checkother.cpp

@@ -220,10 +220,6 @@ void CheckOther::checkAssignmentInAssert()
 //---------------------------------------------------------------------------
 void CheckOther::checkIncorrectLogicOperator()
 {
-    // Inconclusive until #2162 is fixed:
-    if (!_settings->inconclusive)
-        return;
-
     if (!_settings->_checkCodingStyle)
         return;
 
@@ -234,6 +230,7 @@ void CheckOther::checkIncorrectLogicOperator()
     while (tok && endTok)
     {
         // Find a pair of OR'd terms, with or without parenthesis
+        // e.g. if (x != 3 || x != 4)
         const Token *logicTok = NULL, *term1Tok = NULL, *term2Tok = NULL;
         if (NULL != (logicTok = Token::findmatch(tok, "( %any% != %any% ) %oror% ( %any% != %any% ) !!&&", endTok)))
         {
@@ -246,29 +243,55 @@ void CheckOther::checkIncorrectLogicOperator()
             term2Tok = logicTok->tokAt(4);
         }
 
-        // If both terms reference a common variable and are not AND'd with anything, this is an error
+        // The terms must not be AND'd with anything, to prevent false positives
         if (logicTok && (logicTok->strAt(-1) != "&&"))
         {
-            // (var11 != var12) || (var21 != var22)
+            // Find the common variable and the two different-valued constants
-            const unsigned int varId11 = term1Tok->varId();
+            unsigned int variableTested = 0;
-            const unsigned int varId12 = term1Tok->tokAt(2)->varId();
+            std::string firstConstant, secondConstant;
-            const unsigned int varId21 = term2Tok->varId();
+            if (Token::Match(term1Tok, "%var% != %num%"))
-            const unsigned int varId22 = term2Tok->tokAt(2)->varId();
+            {
+                const unsigned int varId = term1Tok->varId();
+                firstConstant = term1Tok->tokAt(2)->str();
 
-            // (var != const1) || (var != const2)
+                if (Token::Match(term2Tok, "%varid% != %num%", varId))
-            if (Token::Match(term1Tok, "%var%") &&
+                {
-                varId11 != 0 &&
+                    variableTested = varId;
-                (varId11 == varId21 || varId11 == varId22))
+                    secondConstant = term2Tok->tokAt(2)->str();
+                }
+                else if (Token::Match(term2Tok, "%num% != %varid%", varId))
+                {
+                    variableTested = varId;
+                    secondConstant = term2Tok->str();
+                }
+            }
+            else if (Token::Match(term1Tok, "%num% != %var%"))
+            {
+                const unsigned int varId = term1Tok->tokAt(2)->varId();
+                firstConstant = term1Tok->str();
+
+                if (Token::Match(term2Tok, "%varid% != %num%", varId))
+                {
+                    variableTested = varId;
+                    secondConstant = term2Tok->tokAt(2)->str();
+                }
+                else if (Token::Match(term2Tok, "%num% != %varid%", varId))
+                {
+                    variableTested = varId;
+                    secondConstant = term2Tok->str();
+                }
+            }
+
+            // If there is a common variable tested for inequality against
+            // either of two different-valued constants, then the expression
+            // will always evaluate to true and the || probably should be an &&
+            if (variableTested != 0 &&
+                !firstConstant.empty() &&
+                !secondConstant.empty() &&
+                firstConstant != secondConstant)
             {
                 incorrectLogicOperatorError(term1Tok);
             }
-            // (const1 != var) || (const2 != var)
-            else if (Token::Match(term1Tok->tokAt(2), "%var%") &&
-                     varId12 != 0 &&
-                     (varId12 == varId21 || varId12 == varId22))
-            {
-                incorrectLogicOperatorError(term1Tok->tokAt(2));
-            }
         }
 
         tok = Token::findmatch(endTok->next(), conditionPattern);
@@ -2643,8 +2666,6 @@ void CheckOther::assignmentInAssertError(const Token *tok, const std::string &va
 
 void CheckOther::incorrectLogicOperatorError(const Token *tok)
 {
-    if (!_settings->inconclusive)
-        return;
     reportError(tok, Severity::warning,
                 "incorrectLogicOperator", "Mutual exclusion over || always evaluates to true. Did you intend to use && instead?");
 }

lib/checkother.h

@@ -251,7 +251,7 @@ public:
                "* look for 'sizeof sizeof ..'\n"
                "* look for calculations inside sizeof()\n"
                "* assignment of a variable to itself\n"
-               //"* mutual exclusion over || always evaluating to true\n"
+               "* mutual exclusion over || always evaluating to true\n"
 
                // optimisations
                "* optimisation: detect post increment/decrement\n";

test/testother.cpp

@@ -104,7 +104,6 @@ private:
         // Check..
         Settings settings;
         settings._checkCodingStyle = true;
-        settings.inconclusive = true;
         CheckOther checkOther(&tokenizer, &settings, this);
 
         // Clear the error buffer..
@@ -1485,6 +1484,16 @@ private:
               "}\n"
              );
         ASSERT_EQUALS("", errout.str());
+
+        check("void f(unsigned int a, unsigned int b, unsigned int c) {\n"
+              "    if((a != b) || (c != b) || (c != a))\n"
+              "    {\n"
+              "        return true;\n"
+              "    }\n"
+              "    return false;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("", errout.str());
     }
 };