From a1cbed3df8894cc7e19b7fe9a352a79744cc0862 Mon Sep 17 00:00:00 2001 From: Zachary Blair Date: Thu, 17 Jan 2013 23:03:04 -0800 Subject: [PATCH] Fixed #4109 (if (c == 1) c == 0; Isn't picked up) --- lib/checkother.cpp | 58 ++++++++++++++++++++++++++++ lib/checkother.h | 7 ++++ test/testother.cpp | 96 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 7c7347e79..a8c39086d 100644 --- a/lib/checkother.cpp +++ b/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; diff --git a/lib/checkother.h b/lib/checkother.h index 41a17a4ef..a09e5a242 100644 --- a/lib/checkother.h +++ b/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" diff --git a/test/testother.cpp b/test/testother.cpp index 5343450f4..46cb92fff 100644 --- a/test/testother.cpp +++ b/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"