From a95c931da000d44c738003b150d31e983e24b60c Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Tue, 5 Jan 2021 05:07:27 -0600 Subject: [PATCH] Fix issue 8501: false negative: (style) Opposite expression on both sides of (#3012) --- lib/astutils.cpp | 62 ++++++++++++++---------------------------- lib/symboldatabase.cpp | 2 +- test/testother.cpp | 30 ++++++++++++++++++-- 3 files changed, 48 insertions(+), 46 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index ffb1c644f..d24485e7c 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -648,6 +648,18 @@ bool exprDependsOnThis(const Token* expr, nonneg int depth) return exprDependsOnThis(expr->astOperand1(), depth) || exprDependsOnThis(expr->astOperand2(), depth); } +static bool hasUnknownVars(const Token* startTok) { + bool result = false; + visitAstNodes(startTok, [&](const Token* tok) { + if (tok->varId() > 0 && !tok->variable()) { + result = true; + return ChildrenToVisit::done; + } + return ChildrenToVisit::op1_and_op2; + }); + return result; +} + /// This takes a token that refers to a variable and it will return the token /// to the expression that the variable is assigned to. If its not valid to /// make such substitution then it will return the original token. @@ -671,11 +683,7 @@ static const Token * followVariableExpression(const Token * tok, bool cpp, const const Token * varTok = getVariableInitExpression(var); if (!varTok) return tok; - // Bailout. If variable value depends on value of "this". - if (exprDependsOnThis(varTok)) - return tok; - // Skip array access - if (Token::simpleMatch(varTok, "[")) + if (hasUnknownVars(varTok)) return tok; if (var->isVolatile()) return tok; @@ -692,44 +700,14 @@ static const Token * followVariableExpression(const Token * tok, bool cpp, const return tok; if (precedes(varTok, endToken) && isAliased(varTok, endToken, tok->varId())) return tok; - if (varTok->exprId() != 0 && isVariableChanged(nextAfterAstRightmostLeaf(varTok), endToken, varTok->exprId(), false, nullptr, cpp)) + const Token* startToken = nextAfterAstRightmostLeaf(varTok); + if (!startToken) + startToken = varTok; + if (varTok->exprId() == 0) { + if (!varTok->isLiteral()) + return tok; + } else if (isExpressionChanged(varTok, startToken, endToken, nullptr, cpp)) { return tok; - // Start at beginning of initialization - const Token * startToken = varTok; - while (Token::Match(startToken, "%op%|.|(|{") && startToken->astOperand1()) - startToken = startToken->astOperand1(); - // Skip if the variable its referring to is modified - for (const Token * tok2 = startToken; tok2 != endToken; tok2 = tok2->next()) { - if (Token::simpleMatch(tok2, ";")) - break; - if (tok2->astParent() && tok2->isUnaryOp("*")) - return tok; - if (tok2->tokType() == Token::eIncDecOp || - tok2->isAssignmentOp() || - Token::Match(tok2, "%name% .|[|++|--|%assign%")) { - return tok; - } - if (Token::Match(tok2, "%name% (")) - // Bailout when function call is seen - return tok; - if (const Variable * var2 = tok2->variable()) { - if (!var2->scope()) - return tok; - const Token * endToken2 = var2->scope() != tok->scope() ? var2->scope()->bodyEnd : endToken; - if (!var2->isLocal() && !var2->isConst() && !var2->isArgument()) - return tok; - if (var2->isStatic() && !var2->isConst()) - return tok; - if (!var2->isConst() && (!precedes(tok2, endToken2) || isVariableChanged(tok2, endToken2, tok2->varId(), false, nullptr, cpp))) - return tok; - if (precedes(tok2, endToken2) && isAliased(tok2, endToken2, tok2->varId())) - return tok; - // Recognized as a variable but the declaration is unknown - } else if (tok2->varId() > 0) { - return tok; - } else if (tok2->tokType() == Token::eName && !Token::Match(tok2, "sizeof|decltype|typeof") && !tok2->function()) { - return tok; - } } return varTok; } diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index f06a1e3d6..49926cf77 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1433,7 +1433,7 @@ void SymbolDatabase::createSymbolDatabaseExprIds() for (Token* tok = const_cast(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) { if (tok->varId() > 0) { tok->exprId(tok->varId()); - } else if (Token::Match(tok, "(|.|%cop%")) { + } else if (Token::Match(tok, "(|.|[|%cop%")) { exprs[tok->str()].push_back(tok); tok->exprId(id++); } diff --git a/test/testother.cpp b/test/testother.cpp index 398e69ed4..d66fda43e 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -4953,13 +4953,13 @@ private: " const int i = sizeof(int);\n" " if ( i != sizeof (int)){}\n" "}\n"); - TODO_ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'i != sizeof(int)' is always false because 'i' and 'sizeof(int)' represent the same value.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'i != sizeof(int)' is always false because 'i' and 'sizeof(int)' represent the same value.\n", errout.str()); check("void f() {\n" " const int i = sizeof(int);\n" " if ( sizeof (int) != i){}\n" "}\n"); - TODO_ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'sizeof(int) != i' is always false because 'sizeof(int)' and 'i' represent the same value.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'sizeof(int) != i' is always false because 'sizeof(int)' and 'i' represent the same value.\n", errout.str()); check("void f(int a = 1) { if ( a != 1){}}\n"); ASSERT_EQUALS("", errout.str()); @@ -5100,7 +5100,7 @@ private: check("struct A { int f() const; };\n" "A g();\n" "void foo() {\n" - " for (const A x = A();;) {\n" + " for (A x = A();;) {\n" " const int a = x.f();\n" " x = g();\n" " if (x.f() == a) break;\n" @@ -5506,6 +5506,30 @@ private: " if (!b && g()) {}\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("void f(bool *a) {\n" + " const bool b = a[42];\n" + " if( b == !(a[42]) ) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Opposite expression on both sides of '=='.\n", errout.str()); + + check("void f(bool *a) {\n" + " const bool b = a[42];\n" + " if( a[42] == !(b) ) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Opposite expression on both sides of '=='.\n", errout.str()); + + check("void f(bool *a) {\n" + " const bool b = *a;\n" + " if( b == !(*a) ) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Opposite expression on both sides of '=='.\n", errout.str()); + + check("void f(bool *a) {\n" + " const bool b = *a;\n" + " if( *a == !(b) ) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Opposite expression on both sides of '=='.\n", errout.str()); } void duplicateVarExpression() {