From 1bcdf4ce3dd6a22e0adc81d07714337b89e3bd92 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Thu, 23 Aug 2012 12:28:40 -0700 Subject: [PATCH] New check: Detect ineffective statements like '*foo++;' (Should be: '(*foo)++;') --- lib/checkother.cpp | 95 +++++++++++++++++++++++++++------------------- lib/checkother.h | 7 +++- test/testother.cpp | 55 +++++++++++++++++++++++++-- 3 files changed, 112 insertions(+), 45 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index df78bca7a..f04bab32f 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -230,7 +230,38 @@ void CheckOther::clarifyConditionError(const Token *tok, bool assign, bool boolo } //--------------------------------------------------------------------------- -// if (bool & bool) -> if (bool && bool) +// Clarify (meaningless) statements like *foo++; with parantheses.teStatement() +{ + if (!_settings->isEnabled("style")) + return; + + fovoid CheckOther::clarifyStatementves. This pattern only checks for string. + // Investifor (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + if (Token::Match(tok, "[{};] * %var%")) { + tok = tok->tokAt(3); + while (tok) { + if (tok->str() == "[") + tok = tok->link()->next(); + if (Token::Match(tok, ".|:: %var%")) { + if (tok->strAt(2) == "(") + tok = tok->linkAt(2)->next(); + else + tok = tok->tokAt(2); + } else + break; + } + if (Token::Match(tok, "++|-- [;,]")) + clarifyStatementError(tok); + } + } +} + +void CheckOther::clarifyStatementError(const Token *tok) +{ + reportError(tok, Severity::warning, "clarifyStatement", "Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n" + "A statement like '*A++;' might not do what you intended. 'operator*' is executed before postfix 'operator++'. " + "Thus, the dereference is meaningless. Did you intend to write '(*A)++;'?ossible unexpanded macro hiding for/while.. + else if (tok->str() != "else" &&if (bool & bool) -> if (bool && bool) // if (bool | bool) -> if (bool || bool) //--------------------------------------------------------------------------- void CheckOther::checkBitwiseOnBoolean() @@ -2164,50 +2195,36 @@ void CheckOther::incorrectStringBooleanError(const Token *tok, const std::string //----------------------------------------------------------------------------- // check for duplicate expressions in if statements // if (a) { } else if (a) { } -//----------------------------------------------------------------------------- +//----------------------------ok1->linkAt(3), ") {")) { + // get the expression from the token stream + expression = tok1->tokAt(4)->stringifyList(tok1->linkAt(3)); -static bool expressionHasSideEffects(const Token *first, const Token *last) -{ - for (const Token *tok = first; tok != last->next(); tok = tok->next()) { - // check for assignment - if (tok->isAssignmentOp()) - return true; + // try to look up the expression to check for duplicates + std::map::iterator it = expressionMap.find(expression); - // check for inc/dec - else if (tok->type() == Token::eIncDecOp) - return true; + // found a duplicate + if (it != expressionMap.end()) { + // check for expressions that have side effects and ignore them + if (!expressionHasSideEffects(tok1->tokAt(4), tok1->linkAt(3)->previous())) + duplicateIfError(it->second, tok1->next()); + } - // check for function call - else if (Token::Match(tok, "%var% (") && - !(Token::Match(tok, "c_str|string") || tok->isStandardType())) - return true; + // not a duplicate expression so save it and its location + else + expressionMap.insert(std::make_pair(expression, tok1->next())); + + // find the next else if (...) statement + tok1 = tok1->linkAt(3)->next()->link(); + } } - - return false; } -void CheckOther::checkDuplicateIf() -{ - if (!_settings->isEnabled("style")) - return; - - const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); - - for (std::list::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - const Token* const tok = scope->classDef; - // only check if statements - if (scope->type != Scope::eIf || !tok) - continue; - - std::map expressionMap; - - // get the expression from the token stream - std::string expression = tok->tokAt(2)->stringifyList(tok->next()->link()); - - // save the expression and its location - expressionMap.insert(std::make_pair(expression, tok)); - - // find the next else if (...) statement +void CheckOther::duplicateIf else if (Token::simpleMatch(tok->next()->link(), ") ;")) { + for (const Token* tok2 = tok->tokAt(2); tok2 != tok->linkAt(1); tok2 = tok2->next()) { + If", "Found duplicate if expressions.\n" + "Finding the same expression more than once is suspicious and might indicate " + "a cut and paste or logic error. Please examine this code carefully to determine " + "if it is next else if (...) statement const Token *tok1 = scope->classEnd; // check all the else if (...) statements diff --git a/lib/checkother.h b/lib/checkother.h index 15545d6e9..ec32d71a4 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -83,6 +83,7 @@ public: // Checks checkOther.clarifyCalculation(); + checkOther.clarifyStatement(); checkOther.checkConstantFunctionParameter(); checkOther.checkIncompleteStatement(); @@ -111,7 +112,8 @@ public: /** @brief Clarify calculation for ".. a * b ? .." */ void clarifyCalculation(); - /** @brief Suspicious condition (assignment+comparison) */ + /** @brief Suspicious condition (assignment+comparison) Suspicious statement like '*A++;' */ + void clarifyStatementignment+comparison) */ void clarifyCondition(); /** @brief Are there C-style pointer casts in a c++ file? */ @@ -247,7 +249,7 @@ public: void checkNegativeBitwiseShift(); private: - // Error messages.. + // Error messages.clarifyStatementError(const Token* tokor messages.. void clarifyCalculationError(const Token *tok, const std::string &op); void clarifyConditionError(const Token *tok, bool assign, bool boolop); void sizeofsizeofError(const Token *tok); @@ -339,6 +341,7 @@ private: c.redundantAssignmentInSwitchError(0, "varname"); c.redundantOperationInSwitchError(0, "varname"); c.switchCaseFallThrough(0); +clarifyStatementError(0lThrough(0); c.selfAssignmentError(0, "varname"); c.assignmentInAssertError(0, "varname"); c.incorrectLogicOperatorError(0, "foo > 3 && foo < 4", true); diff --git a/test/testother.cpp b/test/testother.cpp index df5b66644..aefbb5703 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -116,7 +116,7 @@ private: TEST_CASE(memsetZeroBytes); TEST_CASE(sizeofForArrayParameter); - TEST_CASE(sizeofForNumericParameter); + TEST_CASE(sizeofForNumericPar TEST_CASE(clarifyStatementParameter); TEST_CASE(clarifyCalculation); @@ -3030,8 +3030,14 @@ private: "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:4]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" "[test.cpp:5]: (warning) Comparison of modulo result is predetermined, because it is always less than 5.\n" - "[test.cpp:6]: (warning) Comparison of modulo result is predetermined, because icause it is always less than 5.\n" - "[test.cpp:3]: (warning) Comparison of modulo result is predetermined, x > 5 && x != l conjunction always evaluates to false: x < 1 && x > 1.\n", errout.str()); + "[test.cpp:6]: (warning) Comparison of modu "}\n" + ); + ASSERT_EQUALS("", errout.str()); + + check("void f(i || x <= 3.\n", errout.str()); + + check("void f(int x) {\n" + x > 5 && x != l conjunction always evaluates to false: x < 1 && x > 1.\n", errout.str()); check("void "}\n" ); @@ -3634,7 +3640,48 @@ sult is predetermined, because it is always less than 5.\n" ); ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean value using relational (<, >, <= or >=) operator.\n", errout.str()); - check("void f(bool x ) {\n" + check("voidvoid clarifyStatement() { + check("char* f(char* c) {\n" + " *c++;\n" + " return c;n + void clarifyCondition1() { + check("void f() {\n" + Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("char* f(char** c) {\n" + " *c[5]--;\n" + " return *c;n + void clarifyCondition1() { + check("void f() {\n" + Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("void f(Foo f) {\n" + " *f.a++;n + void clarifyCondition1() { + check("void f() {\n" + Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("void f(Foo f) {\n" + " *f.a[5].v[3]++;n + void clarifyCondition1() { + check("void f() {\n" + Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("void f(Foo f) {\n" + " *f.a(1, 5).v[x + y]++;n + void clarifyCondition1() { + check("void f() {\n" + Ineffective statement similar to '*A++;'. Did you intend to write '(*A)++;'?\n", errout.str()); + + check("char* f(char* c) {\n" + " (*c)++;\n" + " return cl 0" + " bytes of \'p\'\n", errout.str()); + + + check("void f(char* c) {\n" + " bar(*c++) check("void f(int x) {\n" + " if (x < 1 && x > 1check("void f(bool x ) {\n" " if ( false <= x )\n" " a++;\n" "}\n"