Fixed #4109 (if (c == 1) c == 0; Isn't picked up)

lib/checkother.cpp

@@ -1105,6 +1105,64 @@ void CheckOther::suspiciousCaseInSwitchError(const Token* tok, const std::string
                 "Using an operator like '" + operatorString + "' in a case label is suspicious. Did you intend to use a bitwise operator, multiple case labels or if/else instead?", true);
 }
 
+//---------------------------------------------------------------------------
+//    if (x == 1)
+//        x == 0;       // <- suspicious equality comparison.
+//---------------------------------------------------------------------------
+void CheckOther::checkSuspiciousEqualityComparison()
+{
+    if (!_settings->isEnabled("style"))
+        return;
+
+    for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
+
+        if (Token::simpleMatch(tok, "for (")) {
+
+            // Search for any suspicious equality comparison in the initialization
+            // or increment-decrement parts of the for() loop.
+            // For example:
+            //    for (i == 2; i < 10; i++)
+            // or
+            //    for (i = 0; i < 10; i == a)
+            const Token* tok2 = Token::findmatch(tok->next(), "[;(] %var% == %any% [;)]", tok->linkAt(1));
+            if (tok2 && (tok2->str() == "(" || tok2->strAt(4) == ")")) {
+                suspiciousEqualityComparisonError(tok2->tokAt(2));
+            }
+
+            // Equality comparisons with 0 are simplified to negation. For instance,
+            // (x == 0) is simplified to (!x), so also check for suspicious negation
+            // in the initialization or increment-decrement parts of the for() loop.
+            // For example:
+            //    for (!i; i < 10; i++)
+            const Token* tok3 = Token::findmatch(tok->next(), "[;(] ! %var% [;)]", tok->linkAt(1));
+            if (tok3 && (tok3->str() == "(" || tok3->strAt(3) == ")")) {
+                suspiciousEqualityComparisonError(tok3->tokAt(2));
+            }
+
+            // Skip over for() loop conditions because "for (;running==1;)"
+            // is a bit strange, but not necessarily incorrect.
+            tok = tok->linkAt(1);
+        }
+
+        if (Token::Match(tok, "[;{}] *| %var% == %any% ;")) {
+
+            // Exclude compound statements surrounded by parentheses, such as
+            //    printf("%i\n", ({x==0;}));
+            // because they may appear as an expression in GNU C/C++.
+            // See http://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
+            const Token* afterStatement = tok->strAt(1) == "*" ? tok->tokAt(6) : tok->tokAt(5);
+            if (!Token::simpleMatch(afterStatement, "} )"))
+                suspiciousEqualityComparisonError(tok->next());
+        }
+    }
+}
+
+void CheckOther::suspiciousEqualityComparisonError(const Token* tok)
+{
+    reportError(tok, Severity::warning, "suspiciousEqualityComparison",
+                "Found suspicious equality comparison. Did you intend to assign a value instead?", true);
+}
+
 
 //---------------------------------------------------------------------------
 //    int x = 1;

lib/checkother.h

@@ -116,6 +116,7 @@ public:
         checkOther.checkDoubleFree();
         checkOther.checkRedundantCopy();
         checkOther.checkNegativeBitwiseShift();
+        checkOther.checkSuspiciousEqualityComparison();
     }
 
     /** To check the dead code in a program, which is unaccessible due to the counter-conditions check in nested-if statements **/
@@ -191,6 +192,9 @@ public:
     /** @brief %Check for code like 'case A||B:'*/
     void checkSuspiciousCaseInSwitch();
 
+    /** @brief %Check for code like 'case A||B:'*/
+    void checkSuspiciousEqualityComparison();
+
     /** @brief %Check for switch case fall through without comment */
     void checkSwitchCaseFallThrough();
 
@@ -320,6 +324,7 @@ private:
     void redundantBitwiseOperationInSwitchError(const Token *tok, const std::string &varname);
     void switchCaseFallThrough(const Token *tok);
     void suspiciousCaseInSwitchError(const Token* tok, const std::string& operatorString);
+    void suspiciousEqualityComparisonError(const Token* tok);
     void selfAssignmentError(const Token *tok, const std::string &varname);
     void assignmentInAssertError(const Token *tok, const std::string &varname);
     void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always);
@@ -403,6 +408,7 @@ private:
         c.redundantCopyInSwitchError(0, 0, "var");
         c.switchCaseFallThrough(0);
         c.suspiciousCaseInSwitchError(0, "||");
+        c.suspiciousEqualityComparisonError(0);
         c.selfAssignmentError(0, "varname");
         c.assignmentInAssertError(0, "varname");
         c.incorrectLogicOperatorError(0, "foo > 3 && foo < 4", true);
@@ -482,6 +488,7 @@ private:
                "* look for suspicious calculations with sizeof()\n"
                "* assignment of a variable to itself\n"
                "* Suspicious case labels in switch()\n"
+               "* Suspicious equality comparisons\n"
                "* mutual exclusion over || always evaluating to true\n"
                "* Clarify calculation with parentheses\n"
                "* using increment on boolean\n"

test/testother.cpp

@@ -88,6 +88,7 @@ private:
         TEST_CASE(unreachableCode);
 
         TEST_CASE(suspiciousCase);
+        TEST_CASE(suspiciousEqualityComparison);
 
         TEST_CASE(selfAssignment);
         TEST_CASE(trac1132);
@@ -2769,6 +2770,101 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void suspiciousEqualityComparison() {
+        check("void foo(int c) {\n"
+              "    if (c == 1) {\n"
+              "        c == 0;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+
+        check("void foo(int c) {\n"
+              "    if (c == 1) c == 0;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+
+        check("void foo(int* c) {\n"
+              "    if (*c == 1) *c == 0;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+
+
+        check("void foo(int c) {\n"
+              "    if (c == 1) {\n"
+              "        c = 0;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(int c) {\n"
+              "    c == 1;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+
+        check("void foo(int c) {\n"
+              "    for (int i = 0; i == 10; i ++) {\n"
+              "        a ++;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(int c) {\n"
+              "    for (i == 0; i < 10; i ++) {\n"
+              "        c ++;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+
+        check("void foo(int c) {\n"
+              "    for (i == 1; i < 10; i ++) {\n"
+              "        c ++;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+
+        check("void foo(int c) {\n"
+              "    for (i == 2; i < 10; i ++) {\n"
+              "        c ++;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+
+        check("void foo(int c) {\n"
+              "    for (int i = 0; i < 10; i == c) {\n"
+              "        c ++;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+
+        check("void foo(int c) {\n"
+              "    for (; running == 1;) {\n"
+              "        c ++;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(int c) {\n"
+              "    for (; running == 1;) {\n"
+              "        c ++;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(int c) {\n"
+              "    printf(\"%i\n\", ({x==0;}));\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(int x) {\n"
+              "    printf(\"%i\n\", ({int x = do_something(); x == 0;}));\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(int x) {\n"
+              "    printf(\"%i\n\", ({x == 0; x > 0 ? 10 : 20}));\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (warning, inconclusive) Found suspicious equality comparison. Did you intend to assign a value instead?\n", errout.str());
+    }
 
     void selfAssignment() {
         check("void foo()\n"