diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 0cf970aff..66ce1fa7a 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -259,6 +259,55 @@ void CheckOther::checkAssignmentInAssert() } } +//--------------------------------------------------------------------------- +// if ((x != 1) || (x != 3)) // <- always true +//--------------------------------------------------------------------------- +void CheckOther::checkIncorrectLogicOperator() +{ + if (!_settings->_checkCodingStyle) + return; + + const char conditionPattern[] = "if|while ("; + const Token *tok = Token::findmatch(_tokenizer->tokens(), conditionPattern); + const Token *endTok = tok ? tok->next()->link() : NULL; + + while (tok && endTok) + { + // Find a pair of OR'd terms, with or without parenthesis + const Token *logicTok = NULL, *term1Tok = NULL, *term2Tok = NULL; + if ((logicTok = Token::findmatch(tok, "( %any% != %any% ) %oror% ( %any% != %any% ) !!&&", endTok))) + { + term1Tok = logicTok->next(); + term2Tok = logicTok->tokAt(7); + } + else if ((logicTok = Token::findmatch(tok, "%any% != %any% %oror% %any% != %any% !!&&", endTok))) + { + term1Tok = logicTok; + term2Tok = logicTok->tokAt(4); + } + + // If both terms reference a common variable and are not AND'd with anything, this is an error + if (logicTok && (logicTok->strAt(-1) != "&&")) + { + if (Token::Match(term1Tok, "%var%") && + ((term1Tok->str() == term2Tok->str()) || + (term1Tok->str() == term2Tok->strAt(2)))) + { + incorrectLogicOperatorError(term1Tok); + } + else if (Token::Match(term1Tok->tokAt(2), "%var%") && + ((term1Tok->strAt(2) == term2Tok->str()) || + (term1Tok->strAt(2) == term2Tok->strAt(2)))) + { + incorrectLogicOperatorError(term1Tok->tokAt(2)); + } + } + + tok = Token::findmatch(endTok->next(), conditionPattern); + endTok = tok ? tok->next()->link() : NULL; + } +} + //--------------------------------------------------------------------------- // strtol(str, 0, radix) <- radix must be 0 or 2-36 //--------------------------------------------------------------------------- @@ -4174,6 +4223,12 @@ void CheckOther::assignmentInAssertError(const Token *tok, const std::string &va "assignmentInAssert", "Assert statement modifies '" + varname + "'. If the modification is needed in release builds there is a bug."); } +void CheckOther::incorrectLogicOperatorError(const Token *tok) +{ + reportError(tok, Severity::warning, + "incorrectLogicOperator", "Mutual exclusion over || always evaluates to true. Did you intend to use && instead?"); +} + void CheckOther::misusedScopeObjectError(const Token *tok, const std::string& varname) { reportError(tok, Severity::error, diff --git a/lib/checkother.h b/lib/checkother.h index de81d5125..45d92569b 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -84,6 +84,7 @@ public: checkOther.nullConstantDereference(); checkOther.checkSelfAssignment(); + checkOther.checkIncorrectLogicOperator(); // New type of check: Check execution paths checkOther.executionPaths(); @@ -184,6 +185,9 @@ public: /** @brief %Check for assignment to a variable in an assert test*/ void checkAssignmentInAssert(); + /** @brief %Check for testing for mutual exclusion over ||*/ + void checkIncorrectLogicOperator(); + /** @brief %Check for objects that are destroyed immediately */ void checkMisusedScopedObject(); @@ -213,6 +217,7 @@ public: void redundantAssignmentInSwitchError(const Token *tok, const std::string &varname); void selfAssignmentError(const Token *tok, const std::string &varname); void assignmentInAssertError(const Token *tok, const std::string &varname); + void incorrectLogicOperatorError(const Token *tok); void misusedScopeObjectError(const Token *tok, const std::string &varname); void getErrorMessages() @@ -246,6 +251,7 @@ public: selfAssignmentError(0, "varname"); assignmentInAssertError(0, "varname"); invalidScanfError(0); + incorrectLogicOperatorError(0); emptyStringTestError(0, "varname", true); } @@ -285,6 +291,7 @@ public: "* look for 'sizeof sizeof ..'\n" "* look for calculations inside sizeof()\n" "* assignment of a variable to itself\n" + "* mutual exclusion over || always evaluating to true\n" // optimisations "* optimisation: detect post increment/decrement\n" diff --git a/lib/token.cpp b/lib/token.cpp index 9c9114a19..5e1ce97a6 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -454,6 +454,20 @@ bool Token::Match(const Token *tok, const char pattern[], unsigned int varid) p += 5; } + else if (firstWordEquals(p, "%or%") == 0) + { + if (tok->str() != "|") + return false; + p += 4; + } + + else if (firstWordEquals(p, "%oror%") == 0) + { + if (tok->str() != "||") + return false; + p += 6; + } + else if (firstWordEquals(p, tok->_str.c_str())) { p += tok->_str.length(); diff --git a/lib/token.h b/lib/token.h index 1137d2758..eb61846ce 100644 --- a/lib/token.h +++ b/lib/token.h @@ -103,6 +103,8 @@ public: * - "%bool%" true or false * - "%str%" Any token starting with "-character (C-string). * - "%varid%" Match with parameter varid + * - "%or%" A bitwise-or operator '|' + * - "%oror%" A logical-or operator '||' * - "[abc]" Any of the characters 'a' or 'b' or 'c' * - "int|void|char" Any of the strings, int, void or char * - "int|void|char|" Any of the strings, int, void or char or empty string @@ -116,7 +118,6 @@ public: * match its pattern, false is returned. * * @todo pattern "%type%|%num%" should mean either a type or a num. - * @todo pattern "%OR%|%OROR%" should mean either a "|" or a "||" * * @param tok List of tokens to be compared to the pattern * @param pattern The pattern against which the tokens are compared, diff --git a/test/testother.cpp b/test/testother.cpp index 5e5061e80..e6e832299 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -115,6 +115,7 @@ private: TEST_CASE(trac2084); TEST_CASE(assignmentInAssert); + TEST_CASE(incorrectLogicOperator); } void check(const char code[]) @@ -147,6 +148,7 @@ private: checkOther.checkSelfAssignment(); checkOther.invalidScanf(); checkOther.checkMisusedScopedObject(); + checkOther.checkIncorrectLogicOperator(); } @@ -3173,6 +3175,72 @@ private: ); ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'. If the modification is needed in release builds there is a bug.\n", errout.str()); } + + void incorrectLogicOperator() + { + check("void f() {\n" + " if ((x != 1) || (x != 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str()); + + check("void f() {\n" + " if (x != 1 || x != 3)\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str()); + + check("void f() {\n" + " if (1 != x || 3 != x)\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:2]: (warning) Mutual exclusion over || always evaluates to true. Did you intend to use && instead?\n", errout.str()); + + check("void f() {\n" + " if (x != 1 || y != 1)\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " if ((y == 1) && (x != 1) || (x != 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " if ((x != 1) || (x != 3) && (y == 1))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " if ((x != 1) && (x != 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " if ((x == 1) || (x == 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " if ((x != 1) || (y != 3))\n" + " a++;\n" + "}\n" + ); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther) diff --git a/test/testtoken.cpp b/test/testtoken.cpp index 4e05af662..89770dc7e 100644 --- a/test/testtoken.cpp +++ b/test/testtoken.cpp @@ -44,6 +44,7 @@ private: TEST_CASE(matchNothingOrAnyNotElse); TEST_CASE(matchNumeric); TEST_CASE(matchBoolean); + TEST_CASE(matchOr); } void nextprevious() @@ -210,6 +211,16 @@ private: ASSERT_EQUALS(true, Token::Match(negative.tokens(), "%bool%")); } + void matchOr() + { + givenACodeSampleToTokenize bitwiseOr("|"); + ASSERT_EQUALS(true, Token::Match(bitwiseOr.tokens(), "%or%")); + + givenACodeSampleToTokenize logicalOr("||"); + ASSERT_EQUALS(true, Token::Match(logicalOr.tokens(), "%oror%")); + ASSERT_EQUALS(false, Token::Match(logicalOr.tokens(), "%or%")); + ASSERT_EQUALS(false, Token::Match(bitwiseOr.tokens(), "%oror%")); + } class givenACodeSampleToTokenize {