Fixed #2105 (Incorrect operator: mutual exclusion over ||)

lib/checkother.cpp

@@ -259,6 +259,55 @@ void CheckOther::checkAssignmentInAssert()
     }
 }
 
+//---------------------------------------------------------------------------
+//    if ((x != 1) || (x != 3))            // <- always true
+//---------------------------------------------------------------------------
+void CheckOther::checkIncorrectLogicOperator()
+{
+    if (!_settings->_checkCodingStyle)
+        return;
+
+    const char conditionPattern[] = "if|while (";
+    const Token *tok = Token::findmatch(_tokenizer->tokens(), conditionPattern);
+    const Token *endTok = tok ? tok->next()->link() : NULL;
+
+    while (tok && endTok)
+    {
+        // Find a pair of OR'd terms, with or without parenthesis
+        const Token *logicTok = NULL, *term1Tok = NULL, *term2Tok = NULL;
+        if ((logicTok = Token::findmatch(tok, "( %any% != %any% ) %oror% ( %any% != %any% ) !!&&", endTok)))
+        {
+            term1Tok = logicTok->next();
+            term2Tok = logicTok->tokAt(7);
+        }
+        else if ((logicTok = Token::findmatch(tok, "%any% != %any% %oror% %any% != %any% !!&&", endTok)))
+        {
+            term1Tok = logicTok;
+            term2Tok = logicTok->tokAt(4);
+        }
+
+        // If both terms reference a common variable and are not AND'd with anything, this is an error
+        if (logicTok && (logicTok->strAt(-1) != "&&"))
+        {
+            if (Token::Match(term1Tok, "%var%") &&
+                ((term1Tok->str() == term2Tok->str()) ||
+                 (term1Tok->str() == term2Tok->strAt(2))))
+            {
+                incorrectLogicOperatorError(term1Tok);
+            }
+            else if (Token::Match(term1Tok->tokAt(2), "%var%") &&
+                     ((term1Tok->strAt(2) == term2Tok->str()) ||
+                      (term1Tok->strAt(2) == term2Tok->strAt(2))))
+            {
+                incorrectLogicOperatorError(term1Tok->tokAt(2));
+            }
+        }
+
+        tok = Token::findmatch(endTok->next(), conditionPattern);
+        endTok = tok ? tok->next()->link() : NULL;
+    }
+}
+
 //---------------------------------------------------------------------------
 // strtol(str, 0, radix)  <- radix must be 0 or 2-36
 //---------------------------------------------------------------------------
@@ -4174,6 +4223,12 @@ void CheckOther::assignmentInAssertError(const Token *tok, const std::string &va
                 "assignmentInAssert", "Assert statement modifies '" + varname + "'. If the modification is needed in release builds there is a bug.");
 }
 
+void CheckOther::incorrectLogicOperatorError(const Token *tok)
+{
+    reportError(tok, Severity::warning,
+                "incorrectLogicOperator", "Mutual exclusion over || always evaluates to true. Did you intend to use && instead?");
+}
+
 void CheckOther::misusedScopeObjectError(const Token *tok, const std::string& varname)
 {
     reportError(tok, Severity::error,

lib/checkother.h

@@ -84,6 +84,7 @@ public:
 
         checkOther.nullConstantDereference();
         checkOther.checkSelfAssignment();
+        checkOther.checkIncorrectLogicOperator();
 
         // New type of check: Check execution paths
         checkOther.executionPaths();
@@ -184,6 +185,9 @@ public:
     /** @brief %Check for assignment to a variable in an assert test*/
     void checkAssignmentInAssert();
 
+    /** @brief %Check for testing for mutual exclusion over ||*/
+    void checkIncorrectLogicOperator();
+
     /** @brief %Check for objects that are destroyed immediately */
     void checkMisusedScopedObject();
 
@@ -213,6 +217,7 @@ public:
     void redundantAssignmentInSwitchError(const Token *tok, const std::string &varname);
     void selfAssignmentError(const Token *tok, const std::string &varname);
     void assignmentInAssertError(const Token *tok, const std::string &varname);
+    void incorrectLogicOperatorError(const Token *tok);
     void misusedScopeObjectError(const Token *tok, const std::string &varname);
 
     void getErrorMessages()
@@ -246,6 +251,7 @@ public:
         selfAssignmentError(0, "varname");
         assignmentInAssertError(0, "varname");
         invalidScanfError(0);
+        incorrectLogicOperatorError(0);
 
         emptyStringTestError(0, "varname", true);
     }
@@ -285,6 +291,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"
 
                // optimisations
                "* optimisation: detect post increment/decrement\n"

lib/token.cpp

@@ -454,6 +454,20 @@ bool Token::Match(const Token *tok, const char pattern[], unsigned int varid)
                 p += 5;
             }
 
+            else if (firstWordEquals(p, "%or%") == 0)
+            {
+                if (tok->str() != "|")
+                    return false;
+                p += 4;
+            }
+
+            else if (firstWordEquals(p, "%oror%") == 0)
+            {
+                if (tok->str() != "||")
+                    return false;
+                p += 6;
+            }
+
             else if (firstWordEquals(p, tok->_str.c_str()))
             {
                 p += tok->_str.length();

lib/token.h

@@ -103,6 +103,8 @@ public:
      * - "%bool%" true or false
      * - "%str%" Any token starting with "-character (C-string).
      * - "%varid%" Match with parameter varid
+     * - "%or%" A bitwise-or operator '|'
+     * - "%oror%" A logical-or operator '||'
      * - "[abc]" Any of the characters 'a' or 'b' or 'c'
      * - "int|void|char" Any of the strings, int, void or char
      * - "int|void|char|" Any of the strings, int, void or char or empty string
@@ -116,7 +118,6 @@ public:
      * match its pattern, false is returned.
      *
      * @todo pattern "%type%|%num%" should mean either a type or a num.
-     * @todo pattern "%OR%|%OROR%" should mean either a "|" or a "||"
      *
      * @param tok List of tokens to be compared to the pattern
      * @param pattern The pattern against which the tokens are compared,

test/testother.cpp

@@ -115,6 +115,7 @@ private:
         TEST_CASE(trac2084);
 
         TEST_CASE(assignmentInAssert);
+        TEST_CASE(incorrectLogicOperator);
     }
 
     void check(const char code[])
@@ -147,6 +148,7 @@ private:
         checkOther.checkSelfAssignment();
         checkOther.invalidScanf();
         checkOther.checkMisusedScopedObject();
+        checkOther.checkIncorrectLogicOperator();
     }
 
 
@@ -3173,6 +3175,72 @@ private:
              );
         ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'. If the modification is needed in release builds there is a bug.\n", errout.str());
     }
+
+    void incorrectLogicOperator()
+    {
+        check("void f() {\n"
+              "    if ((x != 1) || (x != 3))\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str());
+
+        check("void f() {\n"
+              "    if (x != 1 || x != 3)\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str());
+
+        check("void f() {\n"
+              "    if (1 != x || 3 != x)\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str());
+
+        check("void f() {\n"
+              "    if (x != 1 || y != 1)\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    if ((y == 1) && (x != 1) || (x != 3))\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    if ((x != 1) || (x != 3) && (y == 1))\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    if ((x != 1) && (x != 3))\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    if ((x == 1) || (x == 3))\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "    if ((x != 1) || (y != 3))\n"
+              "        a++;\n"
+              "}\n"
+             );
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)

test/testtoken.cpp

@@ -44,6 +44,7 @@ private:
         TEST_CASE(matchNothingOrAnyNotElse);
         TEST_CASE(matchNumeric);
         TEST_CASE(matchBoolean);
+        TEST_CASE(matchOr);
     }
 
     void nextprevious()
@@ -210,6 +211,16 @@ private:
         ASSERT_EQUALS(true, Token::Match(negative.tokens(), "%bool%"));
     }
 
+    void matchOr()
+    {
+        givenACodeSampleToTokenize bitwiseOr("|");
+        ASSERT_EQUALS(true, Token::Match(bitwiseOr.tokens(), "%or%"));
+
+        givenACodeSampleToTokenize logicalOr("||");
+        ASSERT_EQUALS(true, Token::Match(logicalOr.tokens(), "%oror%"));
+        ASSERT_EQUALS(false, Token::Match(logicalOr.tokens(), "%or%"));
+        ASSERT_EQUALS(false, Token::Match(bitwiseOr.tokens(), "%oror%"));
+    }
 
     class givenACodeSampleToTokenize
     {