From 46645ab327344e3ca60f89fcf3f6a3134ec7291a Mon Sep 17 00:00:00 2001 From: Zachary Blair Date: Mon, 25 Apr 2011 22:45:27 -0700 Subject: [PATCH] Fixed #2722 (new check: statement that is always true (strcmp)) --- lib/checkother.cpp | 52 ++++++++++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 9 +++++++- test/testother.cpp | 48 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index f01626f6b..481ec4279 100644 --- a/lib/checkother.cpp +++ b/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) diff --git a/lib/checkother.h b/lib/checkother.h index ad5e91e5e..ca08afa07 100644 --- a/lib/checkother.h +++ b/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"; diff --git a/test/testother.cpp b/test/testother.cpp index f3d7c3c30..1e30adf07 100644 --- a/test/testother.cpp +++ b/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)