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 18407800aa3..90787a71d4d 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 0b548aa14b4..be6d7730e97 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -2109,10 +2109,10 @@ static void mergeAdjacent(std::list& values) static void removeOverlaps(std::list& 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 b5dc0432eb3..b926f8ebeae 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -2794,16 +2794,14 @@ class TestOther : public TestFixture { "void a(T& x) {\n" " x.dostuff();\n" " const U * y = dynamic_cast(&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(&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 @@ class TestOther : public TestFixture { 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(&x);\n" - " y->mutate();\n" // to avoid warnings that y can be const + " U const * const * * const y = dynamic_cast(&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(&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 @@ class TestOther : public TestFixture { " 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 @@ class TestOther : public TestFixture { 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"