diff --git a/lib/checkother.cpp b/lib/checkother.cpp index fa169935b..a698ecff4 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3230,6 +3230,56 @@ void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2) "carefully to determine if it is correct."); } +//--------------------------------------------------------------------------- +// check for the same expression on both sides of an operator +// (x == x), (x && x), (x || x) +// (x.y == x.y), (x.y && x.y), (x.y || x.y) +//--------------------------------------------------------------------------- + +void CheckOther::checkDuplicateExpression() +{ + if (!_settings->_checkCodingStyle) + return; + + // Parse all executing scopes.. + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + + std::list::const_iterator scope; + + for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) + { + // only check functions + if (scope->type != Scope::eFunction) + continue; + + for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next()) + { + if (Token::Match(tok, "(|&&|%oror% %var% &&|%oror%|==|!=|<=|>=|<|>|-|%or% %var% )|&&|%oror%") && + tok->strAt(1) == tok->strAt(3)) + { + duplicateExpressionError(tok->next(), tok->tokAt(3), tok->strAt(2)); + } + else if (Token::Match(tok, "(|&&|%oror% %var% . %var% &&|%oror%|==|!=|<=|>=|<|>|-|%or% %var% . %var% )|&&|%oror%") && + tok->strAt(1) == tok->strAt(5) && tok->strAt(3) == tok->strAt(7)) + { + duplicateExpressionError(tok->next(), tok->tokAt(6), tok->strAt(4)); + } + } + } +} + +void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string &op) +{ + std::list toks; + toks.push_back(tok2); + toks.push_back(tok1); + + reportError(toks, Severity::style, "duplicateExpression", "Same expression on both sides of \'" + op + "\'.\n" + "Finding the same expression on both sides of an operator is suspicious and might " + "indicate a cut and paste or logic error. Please examine this code carefully to " + "determine if it is correct."); +} + //----------------------------------------------------------------------------- void CheckOther::cstyleCastError(const Token *tok) diff --git a/lib/checkother.h b/lib/checkother.h index 31c12d8d0..ad5e91e5e 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -65,6 +65,7 @@ public: checkOther.checkSelfAssignment(); checkOther.checkDuplicateIf(); checkOther.checkDuplicateBranch(); + checkOther.checkDuplicateExpression(); // information checks checkOther.checkVariableScope(); @@ -212,6 +213,9 @@ public: /** @brief %Check for suspicious code where if and else branch are the same (e.g "if (a) b = true; else b = true;") */ void checkDuplicateBranch(); + /** @brief %Check for suspicious code with the same expression on both sides of operator (e.g "if (a && a)") */ + void checkDuplicateExpression(); + // Error messages.. void cstyleCastError(const Token *tok); void dangerousUsageStrtolError(const Token *tok); @@ -242,6 +246,7 @@ public: void comparisonOfBoolWithIntError(const Token *tok, const std::string &varname); 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 getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) { @@ -288,6 +293,7 @@ public: c.comparisonOfBoolWithIntError(0, "varname"); c.duplicateIfError(0, 0); c.duplicateBranchError(0, 0); + c.duplicateExpressionError(0, 0, "&&"); } std::string myName() const diff --git a/test/testother.cpp b/test/testother.cpp index 395231ff6..e65f6a025 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -115,6 +115,7 @@ private: TEST_CASE(duplicateIf); TEST_CASE(duplicateBranch); + TEST_CASE(duplicateExpression); } void check(const char code[], const char *filename = NULL) @@ -140,6 +141,7 @@ private: checkOther.clarifyCondition(); checkOther.checkDuplicateIf(); checkOther.checkDuplicateBranch(); + checkOther.checkDuplicateExpression(); // Simplify token list.. tokenizer.simplifyTokenList(); @@ -2487,6 +2489,28 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + void duplicateExpression() + { + check("voif foo() {\n" + " if (a == a) { }\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '=='.\n", errout.str()); + + check("void fun() {\n" + " return (a && a ||\n" + " b == b &&\n" + " c - c &&\n" + " d > d &&\n" + " e < e &&\n" + " f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&&'.\n" + "[test.cpp:3] -> [test.cpp:3]: (style) Same expression on both sides of '=='.\n" + "[test.cpp:4] -> [test.cpp:4]: (style) Same expression on both sides of '-'.\n" + "[test.cpp:5] -> [test.cpp:5]: (style) Same expression on both sides of '>'.\n" + "[test.cpp:6] -> [test.cpp:6]: (style) Same expression on both sides of '<'.\n", errout.str()); + } }; REGISTER_TEST(TestOther)