From 83b5cb5b2f1dca5b6eaf22e292143fd2cd242828 Mon Sep 17 00:00:00 2001
From: chrchr-github <78114321+chrchr-github@users.noreply.github.com>
Date: Wed, 22 Nov 2023 14:05:53 +0100
Subject: [PATCH] Fix #12203 false negative: constParameterReference when
 taking address (#5682)

---
 lib/checkother.cpp | 20 +-------------------
 lib/token.cpp      |  4 ++--
 test/testother.cpp | 27 ++++++++++++---------------
 3 files changed, 15 insertions(+), 36 deletions(-)

diff --git a/lib/checkother.cpp b/lib/checkother.cpp
index 18407800a..90787a71d 100644
--- a/lib/checkother.cpp
+++ b/lib/checkother.cpp
@@ -1429,25 +1429,7 @@ void CheckOther::checkConstVariable()
                     }
                 }
                 if (tok->isUnaryOp("&") && Token::Match(tok, "& %varid%", var->declarationId())) {
-                    const Token* opTok = tok->astParent();
-                    int argn = -1;
-                    if (opTok && opTok->isUnaryOp("!"))
-                        continue;
-                    if (opTok && (opTok->isComparisonOp() || opTok->isAssignmentOp() || opTok->isCalculation())) {
-                        if (opTok->isComparisonOp() || opTok->isCalculation()) {
-                            if (opTok->astOperand1() != tok)
-                                opTok = opTok->astOperand1();
-                            else
-                                opTok = opTok->astOperand2();
-                        }
-                        if (opTok && opTok->valueType() && var->valueType() && opTok->valueType()->isConst(var->valueType()->pointer))
-                            continue;
-                    } else if (const Token* ftok = getTokenArgumentFunction(tok, argn)) {
-                        bool inconclusive{};
-                        if (var->valueType() && !isVariableChangedByFunctionCall(ftok, var->valueType()->pointer, var->declarationId(), mSettings, &inconclusive) && !inconclusive)
-                            continue;
-                    }
-                    usedInAssignment = true;
+                    usedInAssignment = isExpressionChangedAt(tok->next(), tok, 0, false, mSettings, true);
                     break;
                 }
                 if (astIsRangeBasedForDecl(tok) && Token::Match(tok->astParent()->astOperand2(), "%varid%", var->declarationId())) {
diff --git a/lib/token.cpp b/lib/token.cpp
index 0b548aa14..be6d7730e 100644
--- a/lib/token.cpp
+++ b/lib/token.cpp
@@ -2109,10 +2109,10 @@ static void mergeAdjacent(std::list<ValueFlow::Value>& values)
 
 static void removeOverlaps(std::list<ValueFlow::Value>& values)
 {
-    for (ValueFlow::Value& x : values) {
+    for (const ValueFlow::Value& x : values) {
         if (x.isNonValue())
             continue;
-        values.remove_if([&](ValueFlow::Value& y) {
+        values.remove_if([&](const ValueFlow::Value& y) {
             if (y.isNonValue())
                 return false;
             if (&x == &y)
diff --git a/test/testother.cpp b/test/testother.cpp
index b5dc0432e..b926f8ebe 100644
--- a/test/testother.cpp
+++ b/test/testother.cpp
@@ -2794,16 +2794,14 @@ private:
               "void a(T& x) {\n"
               "    x.dostuff();\n"
               "    const U * y = dynamic_cast<const U *>(&x);\n"
-              "    y->mutate();\n" // to avoid warnings that y can be const
               "}");
-        TODO_ASSERT_EQUALS("can be const", errout.str(), ""); //Currently taking the address is treated as a non-const operation when it should depend on what we do with it
+        ASSERT_EQUALS("[test.cpp:2]: (style) Parameter 'x' can be declared as reference to const\n", errout.str());
         check("struct T : public U { void dostuff() const {}};\n"
               "void a(T& x) {\n"
               "    x.dostuff();\n"
               "    U const * y = dynamic_cast<U const *>(&x);\n"
-              "    y->mutate();\n" // to avoid warnings that y can be const
               "}");
-        TODO_ASSERT_EQUALS("can be const", errout.str(), ""); //Currently taking the address is treated as a non-const operation when it should depend on what we do with it
+        ASSERT_EQUALS("[test.cpp:2]: (style) Parameter 'x' can be declared as reference to const\n", errout.str());
         check("struct T : public U { void dostuff() const {}};\n"
               "void a(T& x) {\n"
               "    x.dostuff();\n"
@@ -2814,17 +2812,9 @@ private:
         check("struct T : public U { void dostuff() const {}};\n"
               "void a(T& x) {\n"
               "    x.dostuff();\n"
-              "    const U const * const * const * const y = dynamic_cast<const U const * const * const * const>(&x);\n"
-              "    y->mutate();\n" // to avoid warnings that y can be const
+              "    U const * const *  * const y = dynamic_cast<U const * const *  * const>(&x);\n"
               "}");
-        TODO_ASSERT_EQUALS("can be const", errout.str(), ""); //Currently taking the address is treated as a non-const operation when it should depend on what we do with it
-        check("struct T : public U { void dostuff() const {}};\n"
-              "void a(T& x) {\n"
-              "    x.dostuff();\n"
-              "    const U const * const *  * const y = dynamic_cast<const U const * const *  * const>(&x);\n"
-              "    y->mutate();\n" // to avoid warnings that y can be const
-              "}");
-        ASSERT_EQUALS("", errout.str());
+        ASSERT_EQUALS("[test.cpp:2]: (style) Parameter 'x' can be declared as reference to const\n", errout.str());
         check("struct T : public U { void dostuff() const {}};\n"
               "void a(T& x) {\n"
               "    x.dostuff();\n"
@@ -3374,6 +3364,13 @@ private:
               "    return is >> s.x;\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
+
+        check("bool f(std::string& s1, std::string& s2) {\n" // #12203
+              "    return &s1 == &s2;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:1]: (style) Parameter 's1' can be declared as reference to const\n"
+                      "[test.cpp:1]: (style) Parameter 's2' can be declared as reference to const\n",
+                      errout.str());
     }
 
     void constParameterCallback() {
@@ -3763,7 +3760,7 @@ private:
         check("void f(int& i) {\n"
               "    new (&i) int();\n"
               "}\n");
-        ASSERT_EQUALS("", errout.str()); // don't crash
+        TODO_ASSERT_EQUALS("", "[test.cpp:1]: (style) Parameter 'i' can be declared as reference to const\n", errout.str()); // don't crash
 
         check("void f(int& i) {\n"
               "    int& r = i;\n"