From 511ac0ab1f5daf1e7a85277f7f2dfdf4e66e129f Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Sat, 12 Nov 2011 10:23:34 +0100 Subject: [PATCH 1/2] Remove false positives for nested logic --- lib/checkother.cpp | 12 +++++++++--- test/testother.cpp | 9 +++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index a9a4c0aad..db4370b9d 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2405,8 +2405,14 @@ void CheckOther::checkExpressionRange(const Token *start, const Token *end, cons return; Expressions expressions; std::string opName; + int level = 0; for (const Token *tok = start->next(); tok && tok != end; tok = tok->next()) { - if (Token::Match(tok, toCheck.c_str())) { + if (tok->str() == ")") + level--; + else if (tok->str() == "(") + level++; + + if (level == 0 && Token::Match(tok, toCheck.c_str())) { opName = tok->str(); expressions.endExpr(); } else { @@ -2445,7 +2451,7 @@ void CheckOther::complexDuplicateExpressionCheck(const Token *classStart, else if (tok1->str() == "(") level--; - if (level < 0 || Token::Match(tok1, statementStart.c_str())) { + if (level < 0 || (level == 0 && Token::Match(tok1, statementStart.c_str()))) { start = tok1; break; } @@ -2459,7 +2465,7 @@ void CheckOther::complexDuplicateExpressionCheck(const Token *classStart, else if (tok1->str() == "(") level++; - if (level < 0 || Token::Match(tok1, statementEnd.c_str())) { + if (level < 0 || (level == 0 && Token::Match(tok1, statementEnd.c_str()))) { end = tok1; break; } diff --git a/test/testother.cpp b/test/testother.cpp index 06bfa3a82..686bea2cc 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3654,10 +3654,19 @@ private: " if ((a + b) | (a + b)) {}\n" "}"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '|'.\n", errout.str()); + check("void foo() {\n" " if ((a | b) & (a | b)) {}\n" "}"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&'.\n", errout.str()); + + check("void d(const char f, int o, int v)\n" + "{\n" + " if (((f=='R') && (o == 1) && ((v < 2) || (v > 99))) ||\n" + " ((f=='R') && (o == 2) && ((v < 2) || (v > 99))) ||\n" + " ((f=='T') && (o == 2) && ((v < 200) || (v > 9999)))) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void duplicateExpression2() { // ticket #2730 From d28cf42d4c7419c093bbf87906b9af9158e5999e Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Thu, 10 Nov 2011 21:49:14 +0100 Subject: [PATCH 2/2] Fix ticket #3317 (same expression false positives) Add a check for function calls that have no side effects. That means known const methods and a list including strcmp, strlen, etc. If the function is not known to be side effect-free then no style warning is given. Add test cases for the duplicate expressions. --- lib/checkother.cpp | 160 ++++++++++++++++++++++++++++++++++++++------- lib/checkother.h | 10 ++- test/testother.cpp | 64 ++++++++++++++++-- 3 files changed, 203 insertions(+), 31 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index db4370b9d..2b7726926 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2371,35 +2371,141 @@ void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2) } namespace { + struct ExpressionTokens { + const Token *start; + const Token *end; + int count; + ExpressionTokens(const Token *s, const Token *e): start(s), end(e), count(1) {} + }; + class Expressions { public: - void endExpr() { + Expressions(): _start(0) {} + + void endExpr(const Token *end) { const std::string &e = _expression.str(); if (!e.empty()) { - std::map::const_iterator it = _expressions.find(e); + std::map::iterator it = _expressions.find(e); if (it == _expressions.end()) - _expressions[e] = 1; + _expressions.insert(std::make_pair(e, ExpressionTokens(_start, end))); else - _expressions[e] += 1; + it->second.count += 1; } _expression.str(""); + _start = 0; } - void append(const std::string &tok) { - _expression << tok; + void append(const Token *tok) { + if (!_start) + _start = tok; + _expression << tok->str(); } - std::map &getMap() { + std::map &getMap() { return _expressions; } private: - std::map _expressions; + std::map _expressions; std::ostringstream _expression; + const Token *_start; }; + + struct FuncFilter { + FuncFilter(const Scope *scope, const Token *tok): _scope(scope), _tok(tok) {} + + bool operator() (const Function &func) { + // todo: function args, etc?? + bool matchingFunc = func.type == Function::eFunction && + _tok->str() == func.token->str(); + // either a class function, or a global function with the same name + return (_scope && _scope == func.functionScope && matchingFunc) || + (!_scope && matchingFunc); + } + const Scope *_scope; + const Token *_tok; + }; + + + bool inconclusiveFunctionCall(const SymbolDatabase *symbolDatabase, + const std::list &constFunctions, + const ExpressionTokens &tokens) + { + const Token *start = tokens.start; + const Token *end = tokens.end; + // look for function calls between start and end... + for (const Token *tok = start; tok && tok != end; tok = tok->next()) { + if (tok != start && tok->str() == "(") { + // go back to find the function call. + const Token *prev = tok->previous(); + if (prev->str() == ">") { + // ignore template functions like boo() + return true; + } + if (prev && prev->isName()) { + const Variable *v = 0; + if (Token::Match(prev->tokAt(-2), "%var% .")) { + const Token *scope = prev->tokAt(-2); + v = symbolDatabase->getVariableFromVarId(scope->varId()); + } + // hard coded list of safe, no-side-effect functions + if (v == 0 && Token::Match(prev, "strcmp|strncmp|strlen|memcmp|strcasecmp|strncasecmp")) + return false; + std::list::const_iterator it = std::find_if(constFunctions.begin(), + constFunctions.end(), + FuncFilter(v ? v->type(): 0, prev)); + if (it == constFunctions.end()) + return true; + } + } + } + return false; + } + + bool notconst(const Function &func) + { + return !func.isConst; + } + + void getConstFunctions(const SymbolDatabase *symbolDatabase, std::list &constFunctions) + { + std::list::const_iterator scope; + for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { + std::list::const_iterator func; + // only add const functions that do not have a non-const overloaded version + // since it is pretty much impossible to tell which is being called. + typedef std::map > StringFunctionMap; + StringFunctionMap functionsByName; + for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { + StringFunctionMap::iterator it = functionsByName.find(func->token->str()); + Scope *currScope = const_cast(&*scope); + if (it == functionsByName.end()) { + std::list tmp; + tmp.push_back(*func); + tmp.back().functionScope = currScope; + functionsByName[func->token->str()] = tmp; + } else { + it->second.push_back(*func); + it->second.back().functionScope = currScope; + } + } + for (StringFunctionMap::iterator it = functionsByName.begin(); + it != functionsByName.end(); ++it) { + std::list::const_iterator nc = std::find_if(it->second.begin(), it->second.end(), notconst); + if (nc == it->second.end()) { + // ok to add all of them + constFunctions.splice(constFunctions.end(), it->second); + } + } + } + } + } -void CheckOther::checkExpressionRange(const Token *start, const Token *end, const std::string &toCheck) +void CheckOther::checkExpressionRange(const std::list &constFunctions, + const Token *start, + const Token *end, + const std::string &toCheck) { if (!start || !end) return; @@ -2414,22 +2520,27 @@ void CheckOther::checkExpressionRange(const Token *start, const Token *end, cons if (level == 0 && Token::Match(tok, toCheck.c_str())) { opName = tok->str(); - expressions.endExpr(); + expressions.endExpr(tok); } else { - expressions.append(tok->str()); + expressions.append(tok); } } - expressions.endExpr(); - std::map::const_iterator it = expressions.getMap().begin(); + expressions.endExpr(end); + std::map::const_iterator it = expressions.getMap().begin(); + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); for (; it != expressions.getMap().end(); ++it) { - if (it->second > 1) - duplicateExpressionError(start, start, opName); + if (it->second.count > 1 && + (it->first.find("(") == std::string::npos || + !inconclusiveFunctionCall(symbolDatabase, constFunctions, it->second))) { + duplicateExpressionError(it->second.start, it->second.start, opName); + } } } -void CheckOther::complexDuplicateExpressionCheck(const Token *classStart, - const std::string &toCheck, - const std::string &alt) +void CheckOther::complexDuplicateExpressionCheck(const std::list &constFunctions, + const Token *classStart, + const std::string &toCheck, + const std::string &alt) { std::string statementStart(",|=|return"); if (!alt.empty()) @@ -2470,11 +2581,10 @@ void CheckOther::complexDuplicateExpressionCheck(const Token *classStart, break; } } - checkExpressionRange(start, end, toCheck); + checkExpressionRange(constFunctions, start, end, toCheck); } } - //--------------------------------------------------------------------------- // check for the same expression on both sides of an operator // (x == x), (x && x), (x || x) @@ -2489,16 +2599,18 @@ void CheckOther::checkDuplicateExpression() const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); std::list::const_iterator scope; + std::list constFunctions; + getConstFunctions(symbolDatabase, constFunctions); for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { // only check functions if (scope->type != Scope::eFunction) continue; - complexDuplicateExpressionCheck(scope->classStart, "%or%", ""); - complexDuplicateExpressionCheck(scope->classStart, "%oror%", ""); - complexDuplicateExpressionCheck(scope->classStart, "&", "%oror%|%or%"); - complexDuplicateExpressionCheck(scope->classStart, "&&", "%oror%|%or%"); + complexDuplicateExpressionCheck(constFunctions, scope->classStart, "%or%", ""); + complexDuplicateExpressionCheck(constFunctions, scope->classStart, "%oror%", ""); + complexDuplicateExpressionCheck(constFunctions, scope->classStart, "&", "%oror%|%or%"); + complexDuplicateExpressionCheck(constFunctions, scope->classStart, "&&", "%oror%|%or%"); for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next()) { if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% ==|!=|<=|>=|<|>|- %var% )|&&|%oror%|;|,") && diff --git a/lib/checkother.h b/lib/checkother.h index e51a55acd..2d63f87de 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -26,6 +26,7 @@ #include "settings.h" class Token; +class Function; /// @addtogroup Checks /// @{ @@ -434,8 +435,13 @@ private: return varname; } - void checkExpressionRange(const Token *start, const Token *end, const std::string &toCheck); - void complexDuplicateExpressionCheck(const Token *classStart, + void checkExpressionRange(const std::list &constFunctions, + const Token *start, + const Token *end, + const std::string &toCheck); + + void complexDuplicateExpressionCheck(const std::list &constFunctions, + const Token *classStart, const std::string &toCheck, const std::string &alt); }; diff --git a/test/testother.cpp b/test/testother.cpp index 686bea2cc..5385b433a 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -142,6 +142,7 @@ private: TEST_CASE(duplicateBranch); TEST_CASE(duplicateExpression1); TEST_CASE(duplicateExpression2); // ticket #2730 + TEST_CASE(duplicateExpression3); // ticket #3317 TEST_CASE(alwaysTrueFalseStringCompare); TEST_CASE(checkStrncmpSizeof); @@ -3630,11 +3631,6 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str()); - check("void foo() {\n" - " if (this->bar() || this->bar()) {}\n" - "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str()); - check("void foo() {\n" " if (a && b || a && b) {}\n" "}"); @@ -3689,6 +3685,64 @@ private: "[test.cpp:9]: (error) Passing value -1.0 to sqrtf() leads to undefined result\n", errout.str()); } + void duplicateExpression3() { + check("void foo() {\n" + " if (x() || x()) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("struct A {\n" + " void foo() const;\n" + " bool bar() const;\n" + "};\n" + "void A::foo() const {\n" + " if (bar() && bar()) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:6]: (style) Same expression on both sides of '&&'.\n", errout.str()); + + check("struct A {\n" + " void foo();\n" + " bool bar();\n" + " bool bar() const;\n" + "};\n" + "void A::foo() {\n" + " if (bar() && bar()) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("class B {\n" + " void bar(int i);\n" + "};\n" + "class A {\n" + " void bar(int i) const;\n" + "};\n" + "void foo() {\n" + " B b;\n" + " A a;\n" + " if (b.bar(1) && b.bar(1)) {}\n" + " if (a.bar(1) && a.bar(1)) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:11] -> [test.cpp:11]: (style) Same expression on both sides of '&&'.\n", errout.str()); + + check("class D { void strcmp(); };\n" + "void foo() {\n" + " D d;\n" + " if (d.strcmp() && d.strcmp()) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void foo() {\n" + " if ((strcmp(a, b) == 0) || (strcmp(a, b) == 0)) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str()); + + check("void foo() {\n" + " if (str == \"(\" || str == \"(\") {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str()); + } + + void alwaysTrueFalseStringCompare() { check_preprocess_suppress( "#define MACRO \"00FF00\"\n"