From 61ccf888b31f84da6fc7495b2cdf1845e6c23b00 Mon Sep 17 00:00:00 2001
From: Ken-Patrick Lehrmann <kp.lehrmann+github@gmail.com>
Date: Sat, 11 Jul 2020 13:42:57 +0200
Subject: [PATCH] Fix some false positives when the same expression at
 different places does not have the same value

Typically with
```
int F(int *f);
void F2(int *a, int *b) {
  int c = *a;
  F(a); // modifies *a
  if (b && c != *a) {}
}
```
we would get the following FP:
```
[test.cpp:3] -> [test.cpp:5]: (style) The comparison 'c != *a' is always false because 'c' and '*a' represent the same value.\n
```

I guess it boils down to isSameExpression only checking that the
expression is the same (in the above case, "*a" and "*a" are indeed the
same), but there's not real check on the values.
So the patch here is a bit hackish, and we still have false negatives in
cases with dereferenced pointers.
---
 lib/checkother.cpp |  4 +++-
 test/testother.cpp | 29 +++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/lib/checkother.cpp b/lib/checkother.cpp
index d39daaa19..96ed875ee 100644
--- a/lib/checkother.cpp
+++ b/lib/checkother.cpp
@@ -2014,7 +2014,9 @@ void CheckOther::checkDuplicateExpression()
             if (tok->isOp() && tok->astOperand1() && !Token::Match(tok, "+|*|<<|>>|+=|*=|<<=|>>=")) {
                 if (Token::Match(tok, "==|!=|-") && astIsFloat(tok->astOperand1(), true))
                     continue;
-                const bool followVar = !isConstVarExpression(tok) || Token::Match(tok, "%comp%|%oror%|&&");
+                const bool pointerDereference = (tok->astOperand1() && tok->astOperand1()->isUnaryOp("*")) ||
+                                                (tok->astOperand2() && tok->astOperand2()->isUnaryOp("*"));
+                const bool followVar = (!isConstVarExpression(tok) || Token::Match(tok, "%comp%|%oror%|&&")) && !pointerDereference;
                 if (isSameExpression(mTokenizer->isCPP(),
                                      true,
                                      tok->astOperand1(),
diff --git a/test/testother.cpp b/test/testother.cpp
index 773c20a8a..b6463cbac 100644
--- a/test/testother.cpp
+++ b/test/testother.cpp
@@ -243,6 +243,8 @@ private:
         TEST_CASE(unusedVariableValueTemplate); // #8994
 
         TEST_CASE(moduloOfOne);
+
+        TEST_CASE(sameExpressionPointers);
     }
 
     void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, bool verbose=false, Settings* settings = nullptr) {
@@ -5484,6 +5486,24 @@ private:
         ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'val < 0' is always false.\n"
                       "[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'val > 0' is always false.\n", errout.str());
 
+        check("void f() {\n"
+              "  int val = 0;\n"
+              "  int *p = &val;n"
+              "  val = 1;\n"
+              "  if (*p < 0) continue;\n"
+              "  if ((*p > 0)) {}\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f() {\n"
+              "  int val = 0;\n"
+              "  int *p = &val;n"
+              "  if (*p < 0) continue;\n"
+              "  if ((*p > 0)) {}\n"
+              "}\n");
+        TODO_ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) The comparison '*p < 0' is always false.\n"
+                           "[test.cpp:2] -> [test.cpp:4]: (style) The comparison '*p > 0' is always false.\n", "", errout.str());
+
         check("void f() {\n"
               "  int val = 0;\n"
               "  if (val < 0) {\n"
@@ -8701,6 +8721,15 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void sameExpressionPointers() {
+        check("int f(int *i);\n"
+              "void g(int *a, int *b) {\n"
+              "    int c = *a;\n"
+              "    f(a);\n"
+              "    if (b && c != *a) {}\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)