Fix issue 8501: false negative: (style) Opposite expression on both sides of (#3012)

Author: pfultz2

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))
-        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("*"))
+    const Token* startToken = nextAfterAstRightmostLeaf(varTok);
+    if (!startToken)
+        startToken = varTok;
+    if (varTok->exprId() == 0) {
+        if (!varTok->isLiteral())
             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;
-        }
+    } else if (isExpressionChanged(varTok, startToken, endToken, nullptr, cpp)) {
+        return tok;
     }
     return varTok;
 }

lib/symboldatabase.cpp

@@ -1433,7 +1433,7 @@ void SymbolDatabase::createSymbolDatabaseExprIds()
         for (Token* tok = const_cast<Token*>(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++);
             }

test/testother.cpp

@@ -4953,13 +4953,13 @@ class TestOther : public TestFixture {
               "    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 @@ class TestOther : public TestFixture {
         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 @@ class TestOther : public TestFixture {
               "  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() {