From b2eb818d429fe456d2939fc406651f0da831e385 Mon Sep 17 00:00:00 2001 From: chrchr Date: Mon, 20 Nov 2023 10:22:39 +0100 Subject: [PATCH 1/5] Fix #12203 false negative: constParameterReference when taking address --- lib/checkother.cpp | 6 +++--- test/testother.cpp | 7 +++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index abb323ed0d1..4232e59a999 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1431,10 +1431,10 @@ void CheckOther::checkConstVariable() if (tok->isUnaryOp("&") && Token::Match(tok, "& %varid%", var->declarationId())) { const Token* opTok = tok->astParent(); int argn = -1; - if (opTok && opTok->isUnaryOp("!")) + if (opTok && (opTok->isUnaryOp("!") || opTok->isComparisonOp())) continue; - if (opTok && (opTok->isComparisonOp() || opTok->isAssignmentOp() || opTok->isCalculation())) { - if (opTok->isComparisonOp() || opTok->isCalculation()) { + if (opTok && (opTok->isAssignmentOp() || opTok->isCalculation())) { + if (opTok->isCalculation()) { if (opTok->astOperand1() != tok) opTok = opTok->astOperand1(); else diff --git a/test/testother.cpp b/test/testother.cpp index 3997d333a8e..8600a9e3c66 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3374,6 +3374,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() { From 040952e8763a5f4d11017baec2db768b8c4022cc Mon Sep 17 00:00:00 2001 From: chrchr Date: Mon, 20 Nov 2023 11:57:31 +0100 Subject: [PATCH 2/5] Add const --- lib/token.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/token.cpp b/lib/token.cpp index fafaba9ced3..88597b00af4 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -2099,10 +2099,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) From b35ed6ec58b26ac0d8d8ef3127a4f048e4820555 Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 22 Nov 2023 16:42:20 +0100 Subject: [PATCH 3/5] Add test --- test/testother.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/testother.cpp b/test/testother.cpp index 8600a9e3c66..368ae351807 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3381,6 +3381,15 @@ class TestOther : public TestFixture { 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()); + + check("struct S {\n" + " void f(int& r) { p = &r; }\n" + " int* p;\n" + "};\n" + "void g(std::vector& v1, std::vector& v2) {\n" + " std::transform(v1.begin(), v1.end(), v2.begin(), [](auto& x) { return &x; });\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void constParameterCallback() { From a62ab03e90c03db08411749b8ce7b8b07d149700 Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 22 Nov 2023 17:10:00 +0100 Subject: [PATCH 4/5] Undo --- test/testother.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/test/testother.cpp b/test/testother.cpp index 0f1d57848ae..d9868b81673 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -2794,31 +2794,34 @@ 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 "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Parameter 'x' can be declared as reference to const\n", errout.str()); + 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" " U const * y = dynamic_cast(&x);\n" + " y->mutate();\n" // to avoid warnings that y can be const "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Parameter 'x' can be declared as reference to const\n", errout.str()); + 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" - " U * const y = dynamic_cast(&x);\n" + " const U const * const * const * const y = dynamic_cast(&x);\n" " y->mutate();\n" // to avoid warnings that y can be const "}"); ASSERT_EQUALS("", errout.str()); check("struct T : public U { void dostuff() const {}};\n" "void a(T& x) {\n" " x.dostuff();\n" - " U const * const * * const y = dynamic_cast(&x);\n" + " const U const * const * const * const y = dynamic_cast(&x);\n" + " y->mutate();\n" // to avoid warnings that y can be const "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Parameter 'x' can be declared as reference to const\n", errout.str()); + 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" - " my::fancy const * * const y = dynamic_cast const * * const>(&x);\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()); @@ -3769,7 +3772,7 @@ class TestOther : public TestFixture { check("void f(int& i) {\n" " new (&i) int();\n" "}\n"); - TODO_ASSERT_EQUALS("", "[test.cpp:1]: (style) Parameter 'i' can be declared as reference to const\n", errout.str()); // don't crash + ASSERT_EQUALS("", errout.str()); // don't crash check("void f(int& i) {\n" " int& r = i;\n" From 7dc05114e27049d8c04a7ef6f63b412228e27fc9 Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 22 Nov 2023 17:16:03 +0100 Subject: [PATCH 5/5] Format --- test/testother.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/testother.cpp b/test/testother.cpp index d9868b81673..83fad7c3111 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3374,7 +3374,7 @@ class TestOther : public TestFixture { 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()); - + check("struct S {\n" " void f(int& r) { p = &r; }\n" " int* p;\n"