From 4cb97edbaf862683b203b372af56a20d051e825c Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Tue, 8 Nov 2011 21:22:31 +0100 Subject: [PATCH 1/4] 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 From d5664dd6cf05f7f751e36c1f40397212ecc61274 Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Tue, 8 Nov 2011 21:24:44 +0100 Subject: [PATCH 2/4] Improved %or% and & checks --- lib/checkother.cpp | 8 +++++--- test/testother.cpp | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index ba82d3af9..f8ecadec8 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2444,11 +2444,13 @@ void CheckOther::checkDuplicateExpression() if (scope->type != Scope::eFunction) continue; + complexDuplicateExpressionCheck(scope->classStart, "%or%", ""); complexDuplicateExpressionCheck(scope->classStart, "%oror%", ""); - complexDuplicateExpressionCheck(scope->classStart, "&&", "%oror%"); + complexDuplicateExpressionCheck(scope->classStart, "&", "%oror%|%or%"); + complexDuplicateExpressionCheck(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% ==|!=|<=|>=|<|>|-|%or% %var% )|&&|%oror%|;") && + if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% ==|!=|<=|>=|<|>|- %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()) { @@ -2460,7 +2462,7 @@ void CheckOther::checkDuplicateExpression() } duplicateExpressionError(tok->next(), tok->tokAt(3), tok->strAt(2)); - } else if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% . %var% ==|!=|<=|>=|<|>|-|%or% %var% . %var% )|&&|%oror%") && + } else if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% . %var% ==|!=|<=|>=|<|>|- %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/test/testother.cpp b/test/testother.cpp index f1b2d0b2c..17fd241e5 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3456,6 +3456,20 @@ private: " if (a && b || b && c) {}\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("void foo() {\n" + " if (a && b | b && c) {}\n" + "}"); + ASSERT_EQUALS("", 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) & (a | b)) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&'.\n", errout.str()); } void duplicateExpression2() { // ticket #2730 From d1bc8819f9caa65af211092b8350f88c2d1f2c03 Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Tue, 8 Nov 2011 21:26:33 +0100 Subject: [PATCH 3/4] Fix for same expression separated by commas The code branch tested by the previous check for && is now different, so I've changed the test to use == instead. There was also a missing case when the expression was followed by a comma instead of being preceded by one. --- lib/checkother.cpp | 4 ++-- test/testother.cpp | 9 +++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index f8ecadec8..3fb72c52a 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2450,7 +2450,7 @@ void CheckOther::checkDuplicateExpression() complexDuplicateExpressionCheck(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%|;") && + if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% ==|!=|<=|>=|<|>|- %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()) { @@ -2462,7 +2462,7 @@ void CheckOther::checkDuplicateExpression() } duplicateExpressionError(tok->next(), tok->tokAt(3), tok->strAt(2)); - } else if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% . %var% ==|!=|<=|>=|<|>|- %var% . %var% )|&&|%oror%") && + } else if (Token::Match(tok, ",|=|return|(|&&|%oror% %var% . %var% ==|!=|<=|>=|<|>|- %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/test/testother.cpp b/test/testother.cpp index 17fd241e5..54dcf4c7c 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3428,9 +3428,14 @@ private: ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&&'.\n", errout.str()); check("void foo() {\n" - " f(a,b && b);\n" + " f(a,b == b);\n" "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '&&'.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '=='.\n", errout.str()); + + check("void foo() {\n" + " f(b == b, a);\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" From d5c7c5d0987ae902b337ab401a0a8334841e2615 Mon Sep 17 00:00:00 2001 From: Richard Quirk Date: Tue, 8 Nov 2011 21:49:25 +0100 Subject: [PATCH 4/4] Remove duplicate expressions on both sides of || --- lib/mathlib.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/mathlib.cpp b/lib/mathlib.cpp index 7e25e497c..64dc7bda5 100644 --- a/lib/mathlib.cpp +++ b/lib/mathlib.cpp @@ -316,10 +316,10 @@ bool MathLib::isNullValue(const std::string &str) || str == "-0E+00" || str == "+0E+00" || str == "+0E-00" || str == "+0" || str == "+0.0" || str == "+0." - || str == "0.0" || str == "-0e-00" + || str == "0.0" || str == "+0e+00" || str == "-0e+00" || str == "+0e-00" || str == "-0e-00" - || str == "-0E-0" || str == "+0E-00"); + || str == "-0E-0"); } bool MathLib::isOctalDigit(char c)