From 7e403ae2103abb54e8e57741465e3e5068456350 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Sat, 9 Apr 2011 15:14:01 -0400 Subject: [PATCH] fix #311 (add detection of duplicated if else-cases) --- lib/checkother.cpp | 122 +++++++++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 10 +++- test/testother.cpp | 50 +++++++++++++++++++ 3 files changed, 181 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 824c6f2b0..ceaa9501b 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3040,6 +3040,128 @@ void CheckOther::checkIncorrectStringCompare() } } +//----------------------------------------------------------------------------- +// check for duplicate expressions in if statements +// if (a) { } else if (a) { } +//----------------------------------------------------------------------------- + +static const std::string stringifyTokens(const Token *start, const Token *end) +{ + if (start == end) + return ""; + + const Token *tok = start; + std::string stringified = tok->str(); + + while (tok && tok->next() && tok != end) + { + tok = tok->next(); + stringified.append(" "); + stringified.append(tok->str()); + } + + return stringified; +} + +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; + + // check for inc/dec + else if (Token::Match(tok, "++|--")) + return true; + + // check for function call + else if (Token::Match(tok, "%var% (") && + !(Token::Match(tok, "c_str|string") || tok->isStandardType())) + return true; + } + + return false; +} + +void CheckOther::checkDuplicateIf() +{ + if (!_settings->_checkCodingStyle) + return; + + 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; + + // check all the code in the function for if (...) and else if (...) statements + for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next()) + { + if (Token::Match(tok, "if (") && tok->strAt(-1) != "else" && + Token::Match(tok->next()->link(), ") {")) + { + std::map expressionMap; + + // get the expression from the token stream + std::string expression = stringifyTokens(tok->tokAt(2), tok->next()->link()->previous()); + + // save the expression and its location + expressionMap.insert(std::make_pair(expression, tok)); + + // find the next else if (...) statement + const Token *tok1 = tok->next()->link()->next()->link(); + + // check all the else if (...) statements + while (Token::Match(tok1, "} else if (") && + Token::Match(tok1->tokAt(3)->link(), ") {")) + { + // get the expression from the token stream + expression = stringifyTokens(tok1->tokAt(4), tok1->tokAt(3)->link()->previous()); + + // try to look up the expression to check for duplicates + std::map::iterator it = expressionMap.find(expression); + + // found a duplicate + if (it != expressionMap.end()) + { + // check for expressions that have side effects and ignore them + if (!expressionHasSideEffects(tok1->tokAt(4), tok1->tokAt(3)->link()->previous())) + duplicateIfError(it->second, tok1->next()); + } + + // 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->tokAt(3)->link()->next()->link(); + } + + tok = tok->next()->link()->next(); + } + } + } +} + +void CheckOther::duplicateIfError(const Token *tok1, const Token *tok2) +{ + std::list toks; + toks.push_back(tok2); + toks.push_back(tok1); + + reportError(toks, Severity::style, "duplicateIf", "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 correct."); +} + +//----------------------------------------------------------------------------- + void CheckOther::cstyleCastError(const Token *tok) { reportError(tok, Severity::style, "cstyleCast", "C-style pointer casting"); diff --git a/lib/checkother.h b/lib/checkother.h index 8f82ba07f..002e6cbec 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -55,7 +55,6 @@ public: checkOther.checkUnsignedDivision(); checkOther.checkCharVariable(); checkOther.functionVariableUsage(); - checkOther.checkVariableScope(); checkOther.checkStructMemberUsage(); checkOther.strPlusChar(); checkOther.sizeofsizeof(); @@ -64,6 +63,10 @@ public: checkOther.checkAssignmentInAssert(); checkOther.checkSizeofForArrayParameter(); checkOther.checkSelfAssignment(); + checkOther.checkDuplicateIf(); + + // information checks + checkOther.checkVariableScope(); checkOther.clarifyCondition(); // not simplified because ifAssign } @@ -202,6 +205,9 @@ public: /** @brief %Check for suspicious comparison of a bool and a non-zero (and non-one) value (e.g. "if (!x==4)") */ void checkComparisonOfBoolWithInt(); + /** @brief %Check for suspicious code where multiple if have the same expression (e.g "if (a) { } else if (a) { }") */ + void checkDuplicateIf(); + // Error messages.. void cstyleCastError(const Token *tok); void dangerousUsageStrtolError(const Token *tok); @@ -230,6 +236,7 @@ public: void incorrectStringCompareError(const Token *tok, const std::string& func, const std::string &string, const std::string &len); void incrementBooleanError(const Token *tok); void comparisonOfBoolWithIntError(const Token *tok, const std::string &varname); + void duplicateIfError(const Token *tok1, const Token *tok2); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) { @@ -274,6 +281,7 @@ public: c.incorrectStringCompareError(0, "substr", "\"Hello World\"", "12"); c.incrementBooleanError(0); c.comparisonOfBoolWithIntError(0, "varname"); + c.duplicateIfError(0, 0); } std::string myName() const diff --git a/test/testother.cpp b/test/testother.cpp index b1410d6d6..134a0b8ff 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -112,6 +112,8 @@ private: TEST_CASE(incrementBoolean); TEST_CASE(comparisonOfBoolWithInt); + + TEST_CASE(duplicateIf); } void check(const char code[], const char *filename = NULL) @@ -135,6 +137,7 @@ private: checkOther.checkAssignmentInAssert(); checkOther.checkSizeofForArrayParameter(); checkOther.clarifyCondition(); + checkOther.checkDuplicateIf(); // Simplify token list.. tokenizer.simplifyTokenList(); @@ -2397,6 +2400,53 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + void duplicateIf() + { + check("void f(int a, int &b) {\n" + " if (a == 1) { b = 1; }\n" + " else if (a == 2) { b = 2; }\n" + " else if (a == 1) { b = 3; }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2]: (style) Found duplicate if expressions.\n", errout.str()); + + check("void f(int a, int &b) {\n" + " if (a == 1) { b = 1; }\n" + " else if (a == 2) { b = 2; }\n" + " else if (a == 2) { b = 3; }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) Found duplicate if expressions.\n", errout.str()); + + check("void f(int a, int &b) {\n" + " if (a == 1) {\n" + " b = 1;\n" + " if (b == 1) { }\n" + " else if (b == 1) { }\n" + " } else if (a == 2) { b = 2; }\n" + " else if (a == 2) { b = 3; }\n" + "}"); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:6]: (style) Found duplicate if expressions.\n" + "[test.cpp:5] -> [test.cpp:4]: (style) Found duplicate if expressions.\n", errout.str()); + + check("void f(int a, int &b) {\n" + " if (a++) { b = 1; }\n" + " else if (a++) { b = 2; }\n" + " else if (a++) { b = 3; }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int a, int &b) {\n" + " if (!strtok(NULL," ")) { b = 1; }\n" + " else if (!strtok(NULL," ")) { b = 2; }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int a, int &b) {\n" + " if ((x = x / 2) < 100) { b = 1; }\n" + " else if ((x = x / 2) < 100) { b = 2; }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)