From 2e7ec1f34fa1c6b971ce37505f8420ca585d0cf9 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Fri, 19 May 2023 01:02:34 +0200 Subject: [PATCH 01/11] Fix #11720 FN functionConst when using base class members --- lib/checkclass.cpp | 26 +++++++++++++++++++------- test/testclass.cpp | 39 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 5a1d23f439b..a1c3ded7053 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2314,20 +2314,32 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool& return false; memberAccessed = true; } - bool mayModifyArgs = true; + if (const Function* f = funcTok->function()) { // TODO: improve (we bail out if there is any possible modification of any argument) const std::vector args = getArguments(funcTok); const auto argMax = std::min(args.size(), f->argCount()); - mayModifyArgs = false; + for (nonneg int argIndex = 0; argIndex < argMax; ++argIndex) { const Variable* const argVar = f->getArgumentVar(argIndex); - if (!argVar || ((argVar->isArrayOrPointer() || argVar->isReference()) && !argVar->isConst())) { - mayModifyArgs = true; - break; + if (!argVar || ((argVar->isArrayOrPointer() || argVar->isReference()) && !argVar->isConst())) { // argument might be modified + const Token* arg = args[argIndex]; + // member function + if (Token::Match(arg->previous(), "%name% (") && arg->previous()->function() && !arg->previous()->function()->isConst()) + return false; + // Member variable given as parameter + const Token* varTok = previousBeforeAstLeftmostLeaf(arg); + if (!varTok || !(varTok = varTok->next())) + return false; + if ((varTok->isName() && isMemberVar(scope, varTok)) || (varTok->isUnaryOp("&") && (varTok = varTok->astOperand1()) && isMemberVar(scope, varTok))) { + const Variable* var = varTok->variable(); + if (!var || (!var->isMutable() && !var->isConst())) + return false; // TODO: Only bailout if function takes argument as non-const reference + } } } + return true; } - // Member variable given as parameter + const Token *lpar = funcTok->next(); if (Token::simpleMatch(lpar, "( ) (")) lpar = lpar->tokAt(2); @@ -2336,7 +2348,7 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool& tok = tok->link(); else if ((tok->isName() && isMemberVar(scope, tok)) || (tok->isUnaryOp("&") && (tok = tok->astOperand1()) && isMemberVar(scope, tok))) { const Variable* var = tok->variable(); - if ((!var || (!var->isMutable() && !var->isConst())) && mayModifyArgs) + if (!var || (!var->isMutable() && !var->isConst())) return false; // TODO: Only bailout if function takes argument as non-const reference } } diff --git a/test/testclass.cpp b/test/testclass.cpp index 1a5cb02dc45..813fed1a626 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -197,6 +197,7 @@ class TestClass : public TestFixture { TEST_CASE(const84); TEST_CASE(const85); TEST_CASE(const86); + TEST_CASE(const87); TEST_CASE(const_handleDefaultParameters); TEST_CASE(const_passThisToMemberOfOtherClass); @@ -6331,9 +6332,8 @@ class TestClass : public TestFixture { " void h(int, int*) const;\n" " void g() { int a; h(i, &a); }\n" "};\n"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (style, inconclusive) Technically the member function 'S::g' can be const.\n", - "", - errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (style, inconclusive) Technically the member function 'S::g' can be const.\n", + errout.str()); } void const83() { @@ -6398,6 +6398,39 @@ class TestClass : public TestFixture { ASSERT_EQUALS("", errout.str()); } + void const87() { + checkConst("struct Tokenizer {\n" // #11720 + " bool isCPP() const {\n" + " return cpp;\n" + " }\n" + " bool cpp;\n" + "};\n" + "struct Check {\n" + " const Tokenizer* const mTokenizer;\n" + " const int* const mSettings;\n" + "};\n" + "struct CheckA : Check {\n" + " static bool test(const std::string& funcname, const int* settings, bool cpp);\n" + "};\n" + "struct CheckB : Check {\n" + " bool f(const std::string& s);\n" + "};\n" + "bool CheckA::test(const std::string& funcname, const int* settings, bool cpp) {\n" + " return !funcname.empty() && settings && cpp;\n" + "}\n" + "bool CheckB::f(const std::string& s) {\n" + " return CheckA::test(s, mSettings, mTokenizer->isCPP());\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:20] -> [test.cpp:15]: (style, inconclusive) Technically the member function 'CheckB::f' can be const.\n", errout.str()); + + checkConst("void g(int&);\n" + "struct S {\n" + " struct { int i; } a[1];\n" + " void f() { g(a[0].i); }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + } + void const_handleDefaultParameters() { checkConst("struct Foo {\n" " void foo1(int i, int j = 0) {\n" From d19627e0785c2c599f07db642b74a4dc005da0ae Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Fri, 19 May 2023 01:04:46 +0200 Subject: [PATCH 02/11] Format --- lib/checkclass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index a1c3ded7053..72e727364cb 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2339,7 +2339,7 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool& } return true; } - + const Token *lpar = funcTok->next(); if (Token::simpleMatch(lpar, "( ) (")) lpar = lpar->tokAt(2); From bde421ce8dc88363ad1a8076c03d61cee6fdd12f Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Fri, 19 May 2023 01:32:14 +0200 Subject: [PATCH 03/11] Add const --- lib/checkmemoryleak.cpp | 2 +- lib/checkmemoryleak.h | 2 +- lib/reverseanalyzer.cpp | 2 +- lib/symboldatabase.h | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/checkmemoryleak.cpp b/lib/checkmemoryleak.cpp index 2f006fca78e..9c21a0b8ddc 100644 --- a/lib/checkmemoryleak.cpp +++ b/lib/checkmemoryleak.cpp @@ -742,7 +742,7 @@ bool CheckMemoryLeakStructMember::isMalloc(const Variable *variable) return alloc; } -void CheckMemoryLeakStructMember::checkStructVariable(const Variable * const variable) +void CheckMemoryLeakStructMember::checkStructVariable(const Variable* const variable) const { if (!variable) return; diff --git a/lib/checkmemoryleak.h b/lib/checkmemoryleak.h index de78f308c3e..aff9e66c712 100644 --- a/lib/checkmemoryleak.h +++ b/lib/checkmemoryleak.h @@ -284,7 +284,7 @@ class CPPCHECKLIB CheckMemoryLeakStructMember : private Check, private CheckMemo /** Is local variable allocated with malloc? */ static bool isMalloc(const Variable *variable); - void checkStructVariable(const Variable * const variable); + void checkStructVariable(const Variable* const variable) const; void getErrorMessages(ErrorLogger * /*errorLogger*/, const Settings * /*settings*/) const override {} diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index af594200816..277c3a5b557 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -120,7 +120,7 @@ struct ReverseTraversal { return continueB; } - Analyzer::Action analyzeRecursive(const Token* start) { + Analyzer::Action analyzeRecursive(const Token* start) const { Analyzer::Action result = Analyzer::Action::None; visitAstNodes(start, [&](const Token* tok) { result |= analyzer->analyze(tok, Analyzer::Direction::Reverse); diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 739d7d0f8aa..b170a4c6ef1 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -1479,7 +1479,7 @@ class CPPCHECKLIB SymbolDatabase { void createSymbolDatabaseSetTypePointers(); void createSymbolDatabaseSetSmartPointerType(); void createSymbolDatabaseEnums(); // cppcheck-suppress functionConst // has side effects - void createSymbolDatabaseEscapeFunctions(); + void createSymbolDatabaseEscapeFunctions(); // cppcheck-suppress functionConst // has side effects // cppcheck-suppress functionConst void createSymbolDatabaseIncompleteVars(); From 3508dcd84a07d843004ed89d34a903d541522836 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Fri, 19 May 2023 01:50:40 +0200 Subject: [PATCH 04/11] Add const --- lib/checkmemoryleak.cpp | 2 +- lib/checkmemoryleak.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/checkmemoryleak.cpp b/lib/checkmemoryleak.cpp index 9c21a0b8ddc..aec23dceec6 100644 --- a/lib/checkmemoryleak.cpp +++ b/lib/checkmemoryleak.cpp @@ -707,7 +707,7 @@ void CheckMemoryLeakInClass::publicAllocationError(const Token *tok, const std:: } -void CheckMemoryLeakStructMember::check() +void CheckMemoryLeakStructMember::check() const { if (mSettings->clang) return; diff --git a/lib/checkmemoryleak.h b/lib/checkmemoryleak.h index aff9e66c712..0cc9a151065 100644 --- a/lib/checkmemoryleak.h +++ b/lib/checkmemoryleak.h @@ -277,7 +277,7 @@ class CPPCHECKLIB CheckMemoryLeakStructMember : private Check, private CheckMemo checkMemoryLeak.check(); } - void check(); + void check() const; private: From c54610fbaeaa1ca807655e976b4bb57c8fc47959 Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 19 May 2023 11:46:46 +0200 Subject: [PATCH 05/11] Improve const check for arguments, comments, tests --- lib/checkclass.cpp | 13 ++++++------ test/testclass.cpp | 50 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 72e727364cb..d47f6bf9a56 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2315,17 +2315,15 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool& memberAccessed = true; } - if (const Function* f = funcTok->function()) { // TODO: improve (we bail out if there is any possible modification of any argument) + if (const Function* f = funcTok->function()) { // check known function const std::vector args = getArguments(funcTok); const auto argMax = std::min(args.size(), f->argCount()); for (nonneg int argIndex = 0; argIndex < argMax; ++argIndex) { const Variable* const argVar = f->getArgumentVar(argIndex); - if (!argVar || ((argVar->isArrayOrPointer() || argVar->isReference()) && !argVar->isConst())) { // argument might be modified + if (!argVar || ((argVar->isArrayOrPointer() || argVar->isReference()) && + !(argVar->valueType() && argVar->valueType()->isConst(argVar->valueType()->pointer)))) { // argument might be modified const Token* arg = args[argIndex]; - // member function - if (Token::Match(arg->previous(), "%name% (") && arg->previous()->function() && !arg->previous()->function()->isConst()) - return false; // Member variable given as parameter const Token* varTok = previousBeforeAstLeftmostLeaf(arg); if (!varTok || !(varTok = varTok->next())) @@ -2333,13 +2331,14 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool& if ((varTok->isName() && isMemberVar(scope, varTok)) || (varTok->isUnaryOp("&") && (varTok = varTok->astOperand1()) && isMemberVar(scope, varTok))) { const Variable* var = varTok->variable(); if (!var || (!var->isMutable() && !var->isConst())) - return false; // TODO: Only bailout if function takes argument as non-const reference + return false; } } } return true; } + // Member variable given as parameter to unknown function const Token *lpar = funcTok->next(); if (Token::simpleMatch(lpar, "( ) (")) lpar = lpar->tokAt(2); @@ -2349,7 +2348,7 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool& else if ((tok->isName() && isMemberVar(scope, tok)) || (tok->isUnaryOp("&") && (tok = tok->astOperand1()) && isMemberVar(scope, tok))) { const Variable* var = tok->variable(); if (!var || (!var->isMutable() && !var->isConst())) - return false; // TODO: Only bailout if function takes argument as non-const reference + return false; } } return true; diff --git a/test/testclass.cpp b/test/testclass.cpp index 813fed1a626..31857567ba3 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -6025,7 +6025,7 @@ class TestClass : public TestFixture { " int i{};\n" " S f() { return S(&i); }\n" "};\n"); - TODO_ASSERT_EQUALS("[test.cpp:7]: (style, inconclusive) Technically the member function 'C::f' can be const.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (style, inconclusive) Technically the member function 'C::f' can be const.\n", errout.str()); checkConst("struct S {\n" " const int* mp{};\n" @@ -6429,6 +6429,54 @@ class TestClass : public TestFixture { " void f() { g(a[0].i); }\n" "};\n"); ASSERT_EQUALS("", errout.str()); + + checkConst("struct S {\n" + " const int& g() const { return i; }\n" + " int i;\n" + "};\n" + "void h(int, const int&);\n" + "struct T {\n" + " S s;\n" + " int j;\n" + " void f() { h(j, s.g()); }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:9]: (style, inconclusive) Technically the member function 'T::f' can be const.\n", errout.str()); + + checkConst("struct S {\n" + " int& g() { return i; }\n" + " int i;\n" + "};\n" + "void h(int, int&);\n" + "struct T {\n" + " S s;\n" + " int j;\n" + " void f() { h(j, s.g()); }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkConst("struct S {\n" + " const int& g() const { return i; }\n" + " int i;\n" + "};\n" + "void h(int, const int*);\n" + "struct T {\n" + " S s;\n" + " int j;\n" + " void f() { h(j, &s.g()); }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:9]: (style, inconclusive) Technically the member function 'T::f' can be const.\n", errout.str()); + + checkConst("struct S {\n" + " int& g() { return i; }\n" + " int i;\n" + "};\n" + "void h(int, int*);\n" + "struct T {\n" + " S s;\n" + " int j;\n" + " void f() { h(j, &s.g()); }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); } void const_handleDefaultParameters() { From a4d59447375ee17f17e10f5780eece0fb3162fe8 Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 19 May 2023 19:04:06 +0200 Subject: [PATCH 06/11] Add test for #11573 --- test/testother.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/testother.cpp b/test/testother.cpp index 38a3a9bd6a8..b3e92219fe8 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -3632,6 +3632,20 @@ class TestOther : public TestFixture { "[test.cpp:13]: (style) Parameter 's' can be declared as pointer to const\n" "[test.cpp:19]: (style) Parameter 's' can be declared as pointer to const\n", errout.str()); + + check("struct S {\n" // #11573 + " const char* g() const {\n" + " return m;\n" + " }\n" + " const char* m;\n" + "};\n" + "struct T { std::vector v; };\n" + "void f(T* t, const char* n) {\n" + " for (const auto* p : t->v)\n" + " if (strcmp(p->g(), n) == 0) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:8]: (style) Parameter 't' can be declared as pointer to const\n", + errout.str()); } void switchRedundantAssignmentTest() { From 9008ccbe86d064bcee5d0a198930757f5081fb97 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Sat, 20 May 2023 20:24:44 +0200 Subject: [PATCH 07/11] Add test for #11501 --- test/testclass.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/testclass.cpp b/test/testclass.cpp index 31857567ba3..d4368e130a2 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -6316,6 +6316,16 @@ class TestClass : public TestFixture { " void g() { p->f(i); }\n" "};\n"); ASSERT_EQUALS("", errout.str()); + + checkConst("struct A {\n" // #11501 + " enum E { E1 };\n" + " virtual void f(E) const = 0;\n" + "};\n" + "struct F {\n" + " A* a;\n" + " void g() { a->f(A::E1); }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:7]: (style, inconclusive) Technically the member function 'F::g' can be const.\n", errout.str()); } void const82() { // #11513 From 2c51f7a7556cf7de3c34088f59f2d564ec719222 Mon Sep 17 00:00:00 2001 From: chrchr Date: Mon, 22 May 2023 11:49:10 +0200 Subject: [PATCH 08/11] Fix merge --- test/testclass.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/test/testclass.cpp b/test/testclass.cpp index ab419b04358..ad1d824a464 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -6488,6 +6488,7 @@ class TestClass : public TestFixture { " void f() { h(j, &s.g()); }\n" "};\n"); ASSERT_EQUALS("", errout.str()); + } void const88() { // #11626 checkConst("struct S {\n" From cd1e169f990330afc9b55fc04dfa82837a660b86 Mon Sep 17 00:00:00 2001 From: chrchr Date: Tue, 23 May 2023 14:34:50 +0200 Subject: [PATCH 09/11] Add tests --- test/testclass.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/testclass.cpp b/test/testclass.cpp index ad1d824a464..993b3d62b42 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -6488,6 +6488,31 @@ class TestClass : public TestFixture { " void f() { h(j, &s.g()); }\n" "};\n"); ASSERT_EQUALS("", errout.str()); + + checkConst("void j(int** x);\n" + "void k(int* const* y);\n" + "struct S {\n" + " int* p;\n" + " int** q;\n" + " int* const* r;\n" + " void f1() { j(&p); }\n" + " void f2() { j(q); }\n" + " void g1() { k(&p); }\n" + " void g2() { k(q); }\n" + " void g3() { k(r); }\n" + "};\n"); + TODO_ASSERT_EQUALS("f2, g1, g2, g3 can be const", "", errout.str()); + + checkConst("void m(int*& r);\n" + "void n(int* const& s);\n" + "struct T {\n" + " int i;\n" + " int* p;\n" + " void f1() { m(p); }\n" + " void f2() { n(&i); }\n" + " void f3() { n(p); }\n" + "};\n"); + TODO_ASSERT_EQUALS("f3 can be const", "", errout.str()); } void const88() { // #11626 From 713857ccfac8e4202db0152ae6c1aa34267f02eb Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 26 May 2023 14:11:16 +0200 Subject: [PATCH 10/11] Use ASSERT_EQUALS --- test/testclass.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/testclass.cpp b/test/testclass.cpp index 993b3d62b42..f129b262f5e 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -6501,7 +6501,7 @@ class TestClass : public TestFixture { " void g2() { k(q); }\n" " void g3() { k(r); }\n" "};\n"); - TODO_ASSERT_EQUALS("f2, g1, g2, g3 can be const", "", errout.str()); + ASSERT_EQUALS("", errout.str()); checkConst("void m(int*& r);\n" "void n(int* const& s);\n" @@ -6512,7 +6512,7 @@ class TestClass : public TestFixture { " void f2() { n(&i); }\n" " void f3() { n(p); }\n" "};\n"); - TODO_ASSERT_EQUALS("f3 can be const", "", errout.str()); + ASSERT_EQUALS("", errout.str()); } void const88() { // #11626 From f2d711a106a84cd221d1e7ec37959e6a1b4fb83d Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Sat, 27 May 2023 11:08:53 +0200 Subject: [PATCH 11/11] Redundant check --- lib/checkclass.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index b7140c95880..29e61b7c77c 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2328,8 +2328,9 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool& const Token* arg = args[argIndex]; // Member variable given as parameter const Token* varTok = previousBeforeAstLeftmostLeaf(arg); - if (!varTok || !(varTok = varTok->next())) + if (!varTok) return false; + varTok = varTok->next(); if ((varTok->isName() && isMemberVar(scope, varTok)) || (varTok->isUnaryOp("&") && (varTok = varTok->astOperand1()) && isMemberVar(scope, varTok))) { const Variable* var = varTok->variable(); if (!var || (!var->isMutable() && !var->isConst()))