AssignIf: Match lhs and rhs for comparisons. Ticket: #2909

lib/checkassignif.cpp

@@ -32,7 +32,7 @@ CheckAssignIf instance;
 }
 
 
-void CheckAssignIf::check()
+void CheckAssignIf::assignIf()
 {
     if (!_settings->_checkCodingStyle)
         return;
@@ -61,9 +61,9 @@ void CheckAssignIf::check()
                     const std::string op(tok2->strAt(3));
                     const MathLib::bigint num2 = MathLib::toLongNumber(tok2->strAt(4));
                     if (op == "==" && (num & num2) != num2)
-                        mismatchError(tok2, false);
+                        assignIfError(tok2, false);
                     else if (op == "!=" && (num & num2) != num2)
-                        mismatchError(tok2, true);
+                        assignIfError(tok2, true);
                     break;
                 }
             }
@@ -71,9 +71,49 @@ void CheckAssignIf::check()
     }
 }
 
-void CheckAssignIf::mismatchError(const Token *tok, bool result)
+void CheckAssignIf::assignIfError(const Token *tok, bool result)
 {
     reportError(tok, Severity::style,
-                "assignIfMismatchError",
+                "assignIfError",
                 "Mismatching assignment and comparison, comparison is always " + std::string(result ? "true" : "false"));
 }
+
+
+
+
+
+void CheckAssignIf::comparison()
+{
+    if (!_settings->_checkCodingStyle)
+        return;
+
+    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
+    {
+        if (tok->str() != "&")
+            continue;
+
+        if (Token::Match(tok, "& %num% ==|!= %num% &&|%oror%|)"))
+        {
+            const MathLib::bigint num1 = MathLib::toLongNumber(tok->strAt(1));
+            if (num1 < 0)
+                continue;
+
+            const MathLib::bigint num2 = MathLib::toLongNumber(tok->strAt(3));
+            if (num2 < 0)
+                continue;
+
+            if ((num1 & num2) != num2)
+            {
+                const std::string op(tok->strAt(2));
+                comparisonError(tok, op=="==" ? false : true);
+            }
+        }
+    }
+}
+
+void CheckAssignIf::comparisonError(const Token *tok, bool result)
+{
+    reportError(tok, Severity::style,
+                "comparisonError",
+                "Comparison is always " + std::string(result ? "true" : "false"));
+}

lib/checkassignif.h

@@ -50,31 +50,39 @@ public:
     void runSimplifiedChecks(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger)
     {
         CheckAssignIf checkAssignIf(tokenizer, settings, errorLogger);
-        checkAssignIf.check();
+        checkAssignIf.assignIf();
+        checkAssignIf.comparison();
     }
 
-    /** Check for obsolete functions */
-    void check();
+    /** mismatching assignment / comparison */
+    void assignIf();
+
+    /** mismatching lhs and rhs in comparison */
+    void comparison();
 
 private:
 
-    void mismatchError(const Token *tok, bool result);
+    void assignIfError(const Token *tok, bool result);
+    
+    void comparisonError(const Token *tok, bool result);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings)
     {
         CheckAssignIf c(0, settings, errorLogger);
-        c.mismatchError(0, false);
+        c.assignIfError(0, false);
+        c.comparisonError(0, false);
     }
 
     std::string myName() const
     {
-        return "match assignment / conditions";
+        return "match assignments and conditions";
     }
 
     std::string classInfo() const
     {
         return "Match assignments and conditions:\n"
-               " Mismatching assignment and comparison => comparison is always true/false";
+               " * Mismatching assignment and comparison => comparison is always true/false\n"
+               " * Mismatching lhs and rhs in comparison => comparison is always true/false";
     }
 };
 /// @}

test/testassignif.cpp

@@ -35,7 +35,8 @@ private:
 
     void run()
     {
-        TEST_CASE(testand);
+        TEST_CASE(assignAndCompare);
+        TEST_CASE(compare);
     }
 
     void check(const char code[])
@@ -54,11 +55,12 @@ private:
         tokenizer.simplifyTokenList();
 
         // Check char variable usage..
-        CheckAssignIf CheckAssignIf(&tokenizer, &settings, this);
-        CheckAssignIf.check();
+        CheckAssignIf checkAssignIf(&tokenizer, &settings, this);
+        checkAssignIf.assignIf();
+        checkAssignIf.comparison();
     }
 
-    void testand()
+    void assignAndCompare()
     {
         check("void foo(int x)\n"
               "{\n"
@@ -74,6 +76,21 @@ private:
               "}\n");
         ASSERT_EQUALS("[test.cpp:4]: (style) Mismatching assignment and comparison, comparison is always true\n", errout.str());
     }
+
+    void compare()
+    {
+        check("void foo(int x)\n"
+              "{\n"
+              "    if (x & 4 == 3);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Comparison is always false\n", errout.str());
+
+        check("void foo(int x)\n"
+              "{\n"
+              "    if (x & 4 != 3);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (style) Comparison is always true\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestAssignIf)