Skip to content

Commit 6474325

Browse files
chrchr-githubchrchr-github
authored
Fix #11720 FN functionConst when using base class members (#5068)
* Fix #11720 FN functionConst when using base class members * Format * Add const * Add const * Improve const check for arguments, comments, tests * Add test for #11573 * Add test for #11501 * Fix merge * Add tests * Use ASSERT_EQUALS * Redundant check
1 parent 1778977 commit 6474325

7 files changed

Lines changed: 162 additions & 20 deletions

File tree

‎lib/checkclass.cpp‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2316,20 +2316,32 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool&
23162316
return false;
23172317
memberAccessed = true;
23182318
}
2319-
bool mayModifyArgs = true;
2320-
if (const Function* f = funcTok->function()) { // TODO: improve (we bail out if there is any possible modification of any argument)
2319+
2320+
if (const Function* f = funcTok->function()) { // check known function
23212321
const std::vector<const Token*> args = getArguments(funcTok);
23222322
const auto argMax = std::min<nonneg int>(args.size(), f->argCount());
2323-
mayModifyArgs = false;
2323+
23242324
for (nonneg int argIndex = 0; argIndex < argMax; ++argIndex) {
23252325
const Variable* const argVar = f->getArgumentVar(argIndex);
2326-
if (!argVar || ((argVar->isArrayOrPointer() || argVar->isReference()) && !argVar->isConst())) {
2327-
mayModifyArgs = true;
2328-
break;
2326+
if (!argVar || ((argVar->isArrayOrPointer() || argVar->isReference()) &&
2327+
!(argVar->valueType() && argVar->valueType()->isConst(argVar->valueType()->pointer)))) { // argument might be modified
2328+
const Token* arg = args[argIndex];
2329+
// Member variable given as parameter
2330+
const Token* varTok = previousBeforeAstLeftmostLeaf(arg);
2331+
if (!varTok)
2332+
return false;
2333+
varTok = varTok->next();
2334+
if ((varTok->isName() && isMemberVar(scope, varTok)) || (varTok->isUnaryOp("&") && (varTok = varTok->astOperand1()) && isMemberVar(scope, varTok))) {
2335+
const Variable* var = varTok->variable();
2336+
if (!var || (!var->isMutable() && !var->isConst()))
2337+
return false;
2338+
}
23292339
}
23302340
}
2341+
return true;
23312342
}
2332-
// Member variable given as parameter
2343+
2344+
// Member variable given as parameter to unknown function
23332345
const Token *lpar = funcTok->next();
23342346
if (Token::simpleMatch(lpar, "( ) ("))
23352347
lpar = lpar->tokAt(2);
@@ -2338,8 +2350,8 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func, bool&
23382350
tok = tok->link();
23392351
else if ((tok->isName() && isMemberVar(scope, tok)) || (tok->isUnaryOp("&") && (tok = tok->astOperand1()) && isMemberVar(scope, tok))) {
23402352
const Variable* var = tok->variable();
2341-
if ((!var || (!var->isMutable() && !var->isConst())) && mayModifyArgs)
2342-
return false; // TODO: Only bailout if function takes argument as non-const reference
2353+
if (!var || (!var->isMutable() && !var->isConst()))
2354+
return false;
23432355
}
23442356
}
23452357
return true;

‎lib/checkmemoryleak.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -707,7 +707,7 @@ void CheckMemoryLeakInClass::publicAllocationError(const Token *tok, const std::
707707
}
708708

709709

710-
void CheckMemoryLeakStructMember::check()
710+
void CheckMemoryLeakStructMember::check() const
711711
{
712712
if (mSettings->clang)
713713
return;
@@ -740,7 +740,7 @@ bool CheckMemoryLeakStructMember::isMalloc(const Variable *variable)
740740
return alloc;
741741
}
742742

743-
void CheckMemoryLeakStructMember::checkStructVariable(const Variable * const variable)
743+
void CheckMemoryLeakStructMember::checkStructVariable(const Variable* const variable) const
744744
{
745745
if (!variable)
746746
return;

‎lib/checkmemoryleak.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -277,14 +277,14 @@ class CPPCHECKLIB CheckMemoryLeakStructMember : private Check, private CheckMemo
277277
checkMemoryLeak.check();
278278
}
279279

280-
void check();
280+
void check() const;
281281

282282
private:
283283

284284
/** Is local variable allocated with malloc? */
285285
static bool isMalloc(const Variable *variable);
286286

287-
void checkStructVariable(const Variable * const variable);
287+
void checkStructVariable(const Variable* const variable) const;
288288

289289
void getErrorMessages(ErrorLogger * /*errorLogger*/, const Settings * /*settings*/) const override {}
290290

‎lib/reverseanalyzer.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ struct ReverseTraversal {
120120
return continueB;
121121
}
122122

123-
Analyzer::Action analyzeRecursive(const Token* start) {
123+
Analyzer::Action analyzeRecursive(const Token* start) const {
124124
Analyzer::Action result = Analyzer::Action::None;
125125
visitAstNodes(start, [&](const Token* tok) {
126126
result |= analyzer->analyze(tok, Analyzer::Direction::Reverse);

‎lib/symboldatabase.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1481,7 +1481,7 @@ class CPPCHECKLIB SymbolDatabase {
14811481
void createSymbolDatabaseSetTypePointers();
14821482
void createSymbolDatabaseSetSmartPointerType();
14831483
void createSymbolDatabaseEnums(); // cppcheck-suppress functionConst // has side effects
1484-
void createSymbolDatabaseEscapeFunctions();
1484+
void createSymbolDatabaseEscapeFunctions(); // cppcheck-suppress functionConst // has side effects
14851485
// cppcheck-suppress functionConst
14861486
void createSymbolDatabaseIncompleteVars();
14871487

‎test/testclass.cpp‎

Lines changed: 121 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,7 @@ class TestClass : public TestFixture {
198198
TEST_CASE(const85);
199199
TEST_CASE(const86);
200200
TEST_CASE(const87);
201+
TEST_CASE(const88);
201202
TEST_CASE(const89);
202203

203204
TEST_CASE(const_handleDefaultParameters);
@@ -6026,7 +6027,7 @@ class TestClass : public TestFixture {
60266027
" int i{};\n"
60276028
" S f() { return S(&i); }\n"
60286029
"};\n");
6029-
TODO_ASSERT_EQUALS("[test.cpp:7]: (style, inconclusive) Technically the member function 'C::f' can be const.\n", "", errout.str());
6030+
ASSERT_EQUALS("[test.cpp:7]: (style, inconclusive) Technically the member function 'C::f' can be const.\n", errout.str());
60306031

60316032
checkConst("struct S {\n"
60326033
" const int* mp{};\n"
@@ -6317,6 +6318,16 @@ class TestClass : public TestFixture {
63176318
" void g() { p->f(i); }\n"
63186319
"};\n");
63196320
ASSERT_EQUALS("", errout.str());
6321+
6322+
checkConst("struct A {\n" // #11501
6323+
" enum E { E1 };\n"
6324+
" virtual void f(E) const = 0;\n"
6325+
"};\n"
6326+
"struct F {\n"
6327+
" A* a;\n"
6328+
" void g() { a->f(A::E1); }\n"
6329+
"};\n");
6330+
ASSERT_EQUALS("[test.cpp:7]: (style, inconclusive) Technically the member function 'F::g' can be const.\n", errout.str());
63206331
}
63216332

63226333
void const82() { // #11513
@@ -6333,9 +6344,8 @@ class TestClass : public TestFixture {
63336344
" void h(int, int*) const;\n"
63346345
" void g() { int a; h(i, &a); }\n"
63356346
"};\n");
6336-
TODO_ASSERT_EQUALS("[test.cpp:4]: (style, inconclusive) Technically the member function 'S::g' can be const.\n",
6337-
"",
6338-
errout.str());
6347+
ASSERT_EQUALS("[test.cpp:4]: (style, inconclusive) Technically the member function 'S::g' can be const.\n",
6348+
errout.str());
63396349
}
63406350

63416351
void const83() {
@@ -6400,7 +6410,113 @@ class TestClass : public TestFixture {
64006410
ASSERT_EQUALS("", errout.str());
64016411
}
64026412

6403-
void const87() { // #11626
6413+
void const87() {
6414+
checkConst("struct Tokenizer {\n" // #11720
6415+
" bool isCPP() const {\n"
6416+
" return cpp;\n"
6417+
" }\n"
6418+
" bool cpp;\n"
6419+
"};\n"
6420+
"struct Check {\n"
6421+
" const Tokenizer* const mTokenizer;\n"
6422+
" const int* const mSettings;\n"
6423+
"};\n"
6424+
"struct CheckA : Check {\n"
6425+
" static bool test(const std::string& funcname, const int* settings, bool cpp);\n"
6426+
"};\n"
6427+
"struct CheckB : Check {\n"
6428+
" bool f(const std::string& s);\n"
6429+
"};\n"
6430+
"bool CheckA::test(const std::string& funcname, const int* settings, bool cpp) {\n"
6431+
" return !funcname.empty() && settings && cpp;\n"
6432+
"}\n"
6433+
"bool CheckB::f(const std::string& s) {\n"
6434+
" return CheckA::test(s, mSettings, mTokenizer->isCPP());\n"
6435+
"}\n");
6436+
ASSERT_EQUALS("[test.cpp:20] -> [test.cpp:15]: (style, inconclusive) Technically the member function 'CheckB::f' can be const.\n", errout.str());
6437+
6438+
checkConst("void g(int&);\n"
6439+
"struct S {\n"
6440+
" struct { int i; } a[1];\n"
6441+
" void f() { g(a[0].i); }\n"
6442+
"};\n");
6443+
ASSERT_EQUALS("", errout.str());
6444+
6445+
checkConst("struct S {\n"
6446+
" const int& g() const { return i; }\n"
6447+
" int i;\n"
6448+
"};\n"
6449+
"void h(int, const int&);\n"
6450+
"struct T {\n"
6451+
" S s;\n"
6452+
" int j;\n"
6453+
" void f() { h(j, s.g()); }\n"
6454+
"};\n");
6455+
ASSERT_EQUALS("[test.cpp:9]: (style, inconclusive) Technically the member function 'T::f' can be const.\n", errout.str());
6456+
6457+
checkConst("struct S {\n"
6458+
" int& g() { return i; }\n"
6459+
" int i;\n"
6460+
"};\n"
6461+
"void h(int, int&);\n"
6462+
"struct T {\n"
6463+
" S s;\n"
6464+
" int j;\n"
6465+
" void f() { h(j, s.g()); }\n"
6466+
"};\n");
6467+
ASSERT_EQUALS("", errout.str());
6468+
6469+
checkConst("struct S {\n"
6470+
" const int& g() const { return i; }\n"
6471+
" int i;\n"
6472+
"};\n"
6473+
"void h(int, const int*);\n"
6474+
"struct T {\n"
6475+
" S s;\n"
6476+
" int j;\n"
6477+
" void f() { h(j, &s.g()); }\n"
6478+
"};\n");
6479+
ASSERT_EQUALS("[test.cpp:9]: (style, inconclusive) Technically the member function 'T::f' can be const.\n", errout.str());
6480+
6481+
checkConst("struct S {\n"
6482+
" int& g() { return i; }\n"
6483+
" int i;\n"
6484+
"};\n"
6485+
"void h(int, int*);\n"
6486+
"struct T {\n"
6487+
" S s;\n"
6488+
" int j;\n"
6489+
" void f() { h(j, &s.g()); }\n"
6490+
"};\n");
6491+
ASSERT_EQUALS("", errout.str());
6492+
6493+
checkConst("void j(int** x);\n"
6494+
"void k(int* const* y);\n"
6495+
"struct S {\n"
6496+
" int* p;\n"
6497+
" int** q;\n"
6498+
" int* const* r;\n"
6499+
" void f1() { j(&p); }\n"
6500+
" void f2() { j(q); }\n"
6501+
" void g1() { k(&p); }\n"
6502+
" void g2() { k(q); }\n"
6503+
" void g3() { k(r); }\n"
6504+
"};\n");
6505+
ASSERT_EQUALS("", errout.str());
6506+
6507+
checkConst("void m(int*& r);\n"
6508+
"void n(int* const& s);\n"
6509+
"struct T {\n"
6510+
" int i;\n"
6511+
" int* p;\n"
6512+
" void f1() { m(p); }\n"
6513+
" void f2() { n(&i); }\n"
6514+
" void f3() { n(p); }\n"
6515+
"};\n");
6516+
ASSERT_EQUALS("", errout.str());
6517+
}
6518+
6519+
void const88() { // #11626
64046520
checkConst("struct S {\n"
64056521
" bool f() { return static_cast<bool>(p); }\n"
64066522
" const int* g() { return const_cast<const int*>(p); }\n"

‎test/testother.cpp‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3633,6 +3633,20 @@ class TestOther : public TestFixture {
36333633
"[test.cpp:19]: (style) Parameter 's' can be declared as pointer to const\n",
36343634
errout.str());
36353635

3636+
check("struct S {\n" // #11573
3637+
" const char* g() const {\n"
3638+
" return m;\n"
3639+
" }\n"
3640+
" const char* m;\n"
3641+
"};\n"
3642+
"struct T { std::vector<S*> v; };\n"
3643+
"void f(T* t, const char* n) {\n"
3644+
" for (const auto* p : t->v)\n"
3645+
" if (strcmp(p->g(), n) == 0) {}\n"
3646+
"}\n");
3647+
ASSERT_EQUALS("[test.cpp:8]: (style) Parameter 't' can be declared as pointer to const\n",
3648+
errout.str());
3649+
36363650
check("void f(int*& p, int* q) {\n"
36373651
" p = q;\n"
36383652
"}\n");

0 commit comments

Comments
 (0)