Fixed #2722 (new check: statement that is always true (strcmp))

lib/checkother.cpp

@@ -3296,6 +3296,58 @@ void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2,
                 "determine if it is correct.");
 }
 
+
+//---------------------------------------------------------------------------
+// Check for string comparison involving two static strings.
+// if(strcmp("00FF00","00FF00")==0) // <- statement is always true
+//---------------------------------------------------------------------------
+
+void CheckOther::checkAlwaysTrueOrFalseStringCompare()
+{
+    if (!_settings->_checkCodingStyle)
+        return;
+
+    const char pattern1[] = "strcmp|stricmp|strcmpi|strcasecmp|wcscmp ( %str% , %str% )";
+    const char pattern2[] = "QString :: compare ( %str% , %str% )";
+
+    const Token *tok = _tokenizer->tokens();
+    while (tok && (tok = Token::findmatch(tok, pattern1)) != NULL)
+    {
+        alwaysTrueFalseStringCompare(tok, tok->strAt(2), tok->strAt(4));
+        tok = tok->tokAt(5);
+    }
+
+    tok = _tokenizer->tokens();
+    while (tok && (tok = Token::findmatch(tok, pattern2)) != NULL)
+    {
+        alwaysTrueFalseStringCompare(tok, tok->strAt(4), tok->strAt(6));
+        tok = tok->tokAt(7);
+    }
+}
+
+void CheckOther::alwaysTrueFalseStringCompare(const Token *tok, const std::string& str1, const std::string& str2)
+{
+    const size_t stringLen = 10;
+    const std::string string1 = (str1.size() < stringLen) ? str1 : (str1.substr(0, stringLen-2) + "..");
+    const std::string string2 = (str2.size() < stringLen) ? str2 : (str2.substr(0, stringLen-2) + "..");
+
+    if (str1 == str2)
+    {
+        reportError(tok, Severity::warning, "staticStringCompare",
+                    "Comparison of always identical static strings.\n"
+                    "The compared strings, '" + string1 + "' and '" + string2 + "', are always identical. "
+                    "If the purpose is to compare these two strings, the comparison is unnecessary. "
+                    "If the strings are supposed to be different, then there is a bug somewhere.");
+    }
+    else
+    {
+        reportError(tok, Severity::performance, "staticStringCompare",
+                    "Unnecessary comparison of static strings.\n"
+                    "The compared strings, '" + string1 + "' and '" + string2 + "', are static and always different. "
+                    "If the purpose is to compare these two strings, the comparison is unnecessary.");
+    }
+}
+
 //-----------------------------------------------------------------------------
 
 void CheckOther::cstyleCastError(const Token *tok)

lib/checkother.h

@@ -98,6 +98,7 @@ public:
         checkOther.checkIncrementBoolean();
         checkOther.checkComparisonOfBoolWithInt();
         checkOther.checkSwitchCaseFallThrough();
+        checkOther.checkAlwaysTrueOrFalseStringCompare();
     }
 
     /** @brief Clarify calculation for ".. a * b ? .." */
@@ -216,6 +217,9 @@ public:
     /** @brief %Check for suspicious code with the same expression on both sides of operator (e.g "if (a && a)") */
     void checkDuplicateExpression();
 
+    /** @brief %Check for suspicious code that compares string literals for equality */
+    void checkAlwaysTrueOrFalseStringCompare();
+
     // Error messages..
     void cstyleCastError(const Token *tok);
     void dangerousUsageStrtolError(const Token *tok);
@@ -247,6 +251,7 @@ public:
     void duplicateIfError(const Token *tok1, const Token *tok2);
     void duplicateBranchError(const Token *tok1, const Token *tok2);
     void duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string &op);
+    void alwaysTrueFalseStringCompare(const Token *tok, const std::string& str1, const std::string& str2);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings)
     {
@@ -294,6 +299,7 @@ public:
         c.duplicateIfError(0, 0);
         c.duplicateBranchError(0, 0);
         c.duplicateExpressionError(0, 0, "&&");
+        c.alwaysTrueFalseStringCompare(0, "str1", "str2");
     }
 
     std::string myName() const
@@ -336,7 +342,8 @@ public:
                "* Clarify calculation with parentheses\n"
                "* using increment on boolean\n"
                "* comparison of a boolean with a non-zero integer\n"
-               "* suspicious condition (assignment+comparison)"
+               "* suspicious condition (assignment+comparison)\n"
+               "* suspicious condition (runtime comparison of string literals)\n"
 
                // optimisations
                "* optimisation: detect post increment/decrement\n";

test/testother.cpp

@@ -117,6 +117,8 @@ private:
         TEST_CASE(duplicateBranch);
         TEST_CASE(duplicateExpression1);
         TEST_CASE(duplicateExpression2); // ticket #2730
+
+        TEST_CASE(alwaysTrueFalseStringCompare);
     }
 
     void check(const char code[], const char *filename = NULL)
@@ -216,6 +218,7 @@ private:
         // Check..
         CheckOther checkOther(&tokenizer, &settings, &logger);
         checkOther.checkSwitchCaseFallThrough();
+        checkOther.checkAlwaysTrueOrFalseStringCompare();
 
         logger.reportUnmatchedSuppressions(settings.nomsg.getUnmatchedLocalSuppressions(filename));
     }
@@ -2549,6 +2552,51 @@ private:
                       "[test.cpp:8]: (error) Passing value -1.0 to sqrt() leads to undefined result\n"
                       "[test.cpp:9]: (error) Passing value -1.0 to sqrtf() leads to undefined result\n", errout.str());
     }
+
+    void alwaysTrueFalseStringCompare()
+    {
+        check_preprocess_suppress(
+            "#define MACRO \"00FF00\"\n"
+            "int main()\n"
+            "{\n"
+            "  if (strcmp(MACRO,\"00FF00\") == 0)"
+            "  {"
+            "    std::cout << \"Equal\n\""
+            "  }"
+            "}");
+        ASSERT_EQUALS("[test.cpp:4]: (warning) Comparison of always identical static strings.\n", errout.str());
+
+        check_preprocess_suppress(
+            "int main()\n"
+            "{\n"
+            "  if (stricmp(\"hotdog\",\"HOTdog\") == 0)"
+            "  {"
+            "    std::cout << \"Equal\n\""
+            "  }"
+            "}");
+        ASSERT_EQUALS("[test.cpp:3]: (performance) Unnecessary comparison of static strings.\n", errout.str());
+
+        check_preprocess_suppress(
+            "#define MACRO \"Hotdog\"\n"
+            "int main()\n"
+            "{\n"
+            "  if (QString::compare(\"Hamburger\", MACRO) == 0)"
+            "  {"
+            "    std::cout << \"Equal\n\""
+            "  }"
+            "}");
+        ASSERT_EQUALS("[test.cpp:4]: (performance) Unnecessary comparison of static strings.\n", errout.str());
+
+        check_preprocess_suppress(
+            "int main()\n"
+            "{\n"
+            "  if (QString::compare(argv[2], \"hotdog\") == 0)"
+            "  {"
+            "    std::cout << \"Equal\n\""
+            "  }"
+            "}");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)