From 4cb97edbaf862683b203b372af56a20d051e825c Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Tue, 8 Nov 2011 21:22:31 +0100 Subject: [PATCH] Improved same expression check for ticket #3274 Expand the logic for the check for the same expression on both sides of the || and && operators. Now expressions can be more complex, with the "alt" variable helping to fudge operator precedence to avoid false positives. --- lib/checkother.cpp | 106 ++++++++++++++++++++++++++++++++++++++++++++- lib/checkother.h | 5 +++ test/testother.cpp | 27 +++++++++++- 3 files changed, 135 insertions(+), 3 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index cb95750f5..ba82d3af9 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2325,6 +2325,105 @@ void CheckOther::duplicateBranchError(const Token *tok1, const Token *tok2) "carefully to determine if it is correct."); } +namespace { + class Expressions { + public: + void endExpr() { + const std::string &e = _expression.str(); + if (!e.empty()) { + std::map::const_iterator it = _expressions.find(e); + if (it == _expressions.end()) + _expressions[e] = 1; + else + _expressions[e] += 1; + } + _expression.str(""); + } + + void append(const std::string &tok) { + _expression << tok; + } + + std::map &getMap() { + return _expressions; + } + + private: + std::map _expressions; + std::ostringstream _expression; + }; +} + +void CheckOther::checkExpressionRange(const Token *start, const Token *end, const std::string &toCheck) +{ + if (!start || !end) + return; + Expressions expressions; + std::string opName; + for (const Token *tok = start->next(); tok && tok != end; tok = tok->next()) { + if (Token::Match(tok, toCheck.c_str())) { + opName = tok->str(); + expressions.endExpr(); + } else { + expressions.append(tok->str()); + } + } + expressions.endExpr(); + std::map::const_iterator it = expressions.getMap().begin(); + for (; it != expressions.getMap().end(); ++it) { + if (it->second > 1) + duplicateExpressionError(start, start, opName); + } +} + +void CheckOther::complexDuplicateExpressionCheck(const Token *classStart, + const std::string &toCheck, + const std::string &alt) +{ + std::string statementStart(",|=|return"); + if (!alt.empty()) + statementStart += "|" + alt; + std::string statementEnd(";|,"); + if (!alt.empty()) + statementEnd += "|" + alt; + + for (const Token *tok = classStart; tok && tok != classStart->link(); tok = tok->next()) { + if (!Token::Match(tok, toCheck.c_str())) + continue; + + // look backward for the start of the statement + const Token *start = 0; + int level = 0; + for (const Token *tok1 = tok->previous(); tok1 && tok1 != classStart; tok1 = tok1->previous()) { + if (tok1->str() == ")") + level++; + else if (tok1->str() == "(") + level--; + + if (level < 0 || Token::Match(tok1, statementStart.c_str())) { + start = tok1; + break; + } + } + const Token *end = 0; + level = 0; + // look for the end of the statement + for (const Token *tok1 = tok->next(); tok1 && tok1 != classStart->link(); tok1 = tok1->next()) { + if (tok1->str() == ")") + level--; + else if (tok1->str() == "(") + level++; + + if (level < 0 || Token::Match(tok1, statementEnd.c_str())) { + end = tok1; + break; + } + } + checkExpressionRange(start, end, toCheck); + } +} + + //--------------------------------------------------------------------------- // check for the same expression on both sides of an operator // (x == x), (x && x), (x || x) @@ -2345,8 +2444,11 @@ void CheckOther::checkDuplicateExpression() if (scope->type != Scope::eFunction) continue; + complexDuplicateExpressionCheck(scope->classStart, "%oror%", ""); + complexDuplicateExpressionCheck(scope->classStart, "&&", "%oror%"); + for (const Token *tok = scope->classStart; tok && tok != scope->classStart->link(); tok = tok->next()) { - if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% &&|%oror%|==|!=|<=|>=|<|>|-|%or% %var% )|&&|%oror%|;") && + if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% ==|!=|<=|>=|<|>|-|%or% %var% )|&&|%oror%|;") && tok->strAt(1) == tok->strAt(3)) { // float == float and float != float are valid NaN checks if (Token::Match(tok->tokAt(2), "==|!=") && tok->next()->varId()) { @@ -2358,7 +2460,7 @@ void CheckOther::checkDuplicateExpression() } duplicateExpressionError(tok->next(), tok->tokAt(3), tok->strAt(2)); - } else if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% . %var% &&|%oror%|==|!=|<=|>=|<|>|-|%or% %var% . %var% )|&&|%oror%") && + } else if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% . %var% ==|!=|<=|>=|<|>|-|%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)); } diff --git a/lib/checkother.h b/lib/checkother.h index 11b99f106..caf6ad42c 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -426,6 +426,11 @@ private: return varname; } + + void checkExpressionRange(const Token *start, const Token *end, const std::string &toCheck); + void complexDuplicateExpressionCheck(const Token *classStart, + const std::string &toCheck, + const std::string &alt); }; /// @} //--------------------------------------------------------------------------- diff --git a/test/testother.cpp b/test/testother.cpp index 99337c828..f1b2d0b2c 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -2242,7 +2242,7 @@ private: " a++;\n" "}\n" ); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&&'.\n", errout.str()); check("void f(int x) {\n" " if (x == 1 && x == 3)\n" @@ -3431,6 +3431,31 @@ private: " f(a,b && 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 (x!=2 || x!=2) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str()); + + check("void foo() {\n" + " if (x!=2 || x!=3 || x!=2) {}\n" + "}"); + 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" + "}"); + 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 || b && c) {}\n" + "}"); + ASSERT_EQUALS("", errout.str()); } void duplicateExpression2() { // ticket #2730