Skip to content

Commit cf38a65

Browse files
ludviggunneludviggunne
authored
fix #13915: Improve check: copy constructor and assignment operator should be defined when class deletes pointer (danmar#7703)
1 parent b4a1e77 commit cf38a65

File tree

3 files changed

+88
-32
lines changed

3 files changed

+88
-32
lines changed

‎lib/checkclass.cpp

Lines changed: 47 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -413,30 +413,47 @@ void CheckClass::copyconstructors()
413413

414414
for (const Scope * scope : mSymbolDatabase->classAndStructScopes) {
415415
std::map<int, const Token*> allocatedVars;
416+
std::map<int, const Token*> deallocatedVars;
416417

417418
for (const Function &func : scope->functionList) {
418-
if (func.type != FunctionType::eConstructor || !func.functionScope)
419-
continue;
420-
const Token* tok = func.token->linkAt(1);
421-
for (const Token* const end = func.functionScope->bodyStart; tok != end; tok = tok->next()) {
422-
if (Token::Match(tok, "%var% ( new") ||
423-
(Token::Match(tok, "%var% ( %name% (") && mSettings->library.getAllocFuncInfo(tok->tokAt(2)))) {
424-
const Variable* var = tok->variable();
425-
if (var && var->isPointer() && var->scope() == scope)
426-
allocatedVars[tok->varId()] = tok;
419+
if (func.type == FunctionType::eConstructor && func.functionScope) {
420+
// Allocations in constructors
421+
const Token* tok = func.token->linkAt(1);
422+
for (const Token* const end = func.functionScope->bodyStart; tok != end; tok = tok->next()) {
423+
if (Token::Match(tok, "%var% ( new") ||
424+
(Token::Match(tok, "%var% ( %name% (") && mSettings->library.getAllocFuncInfo(tok->tokAt(2)))) {
425+
const Variable* var = tok->variable();
426+
if (var && var->isPointer() && var->scope() == scope)
427+
allocatedVars[tok->varId()] = tok;
428+
}
427429
}
428-
}
429-
for (const Token* const end = func.functionScope->bodyEnd; tok != end; tok = tok->next()) {
430-
if (Token::Match(tok, "%var% = new") ||
431-
(Token::Match(tok, "%var% = %name% (") && mSettings->library.getAllocFuncInfo(tok->tokAt(2)))) {
432-
const Variable* var = tok->variable();
433-
if (var && var->isPointer() && var->scope() == scope && !var->isStatic())
434-
allocatedVars[tok->varId()] = tok;
430+
for (const Token* const end = func.functionScope->bodyEnd; tok != end; tok = tok->next()) {
431+
if (Token::Match(tok, "%var% = new") ||
432+
(Token::Match(tok, "%var% = %name% (") && mSettings->library.getAllocFuncInfo(tok->tokAt(2)))) {
433+
const Variable* var = tok->variable();
434+
if (var && var->isPointer() && var->scope() == scope && !var->isStatic())
435+
allocatedVars[tok->varId()] = tok;
436+
}
437+
}
438+
} else if (func.type == FunctionType::eDestructor && func.functionScope) {
439+
// Deallocations in destructors
440+
const Token* tok = func.functionScope->bodyStart;
441+
for (const Token* const end = func.functionScope->bodyEnd; tok != end; tok = tok->next()) {
442+
if (Token::Match(tok, "delete %var%") ||
443+
(Token::Match(tok, "%name% ( %var%") && mSettings->library.getDeallocFuncInfo(tok))) {
444+
const Token *vartok = tok->str() == "delete" ? tok->next() : tok->tokAt(2);
445+
const Variable* var = vartok->variable();
446+
if (var && var->isPointer() && var->scope() == scope && !var->isStatic())
447+
deallocatedVars[vartok->varId()] = vartok;
448+
}
435449
}
436450
}
437451
}
438452

439-
if (!allocatedVars.empty()) {
453+
const bool hasAllocatedVars = !allocatedVars.empty();
454+
const bool hasDeallocatedVars = !deallocatedVars.empty();
455+
456+
if (hasAllocatedVars || hasDeallocatedVars) {
440457
const Function *funcCopyCtor = nullptr;
441458
const Function *funcOperatorEq = nullptr;
442459
const Function *funcDestructor = nullptr;
@@ -450,13 +467,21 @@ void CheckClass::copyconstructors()
450467
}
451468
if (!funcCopyCtor || funcCopyCtor->isDefault()) {
452469
bool unknown = false;
453-
if (!hasNonCopyableBase(scope, &unknown) && !unknown)
454-
noCopyConstructorError(scope, funcCopyCtor, allocatedVars.cbegin()->second, unknown);
470+
if (!hasNonCopyableBase(scope, &unknown) && !unknown) {
471+
if (hasAllocatedVars)
472+
noCopyConstructorError(scope, funcCopyCtor, allocatedVars.cbegin()->second, unknown);
473+
else
474+
noCopyConstructorError(scope, funcCopyCtor, deallocatedVars.cbegin()->second, unknown);
475+
}
455476
}
456477
if (!funcOperatorEq || funcOperatorEq->isDefault()) {
457478
bool unknown = false;
458-
if (!hasNonCopyableBase(scope, &unknown) && !unknown)
459-
noOperatorEqError(scope, funcOperatorEq, allocatedVars.cbegin()->second, unknown);
479+
if (!hasNonCopyableBase(scope, &unknown) && !unknown) {
480+
if (hasAllocatedVars)
481+
noOperatorEqError(scope, funcOperatorEq, allocatedVars.cbegin()->second, unknown);
482+
else
483+
noOperatorEqError(scope, funcOperatorEq, deallocatedVars.cbegin()->second, unknown);
484+
}
460485
}
461486
if (!funcDestructor || funcDestructor->isDefault()) {
462487
const Token * mustDealloc = nullptr;
@@ -556,7 +581,7 @@ static std::string noMemberErrorMessage(const Scope *scope, const char function[
556581
else
557582
errmsg += " It is recommended to define or delete the " + std::string(function) + '.';
558583
} else {
559-
errmsg += type + " '$symbol' does not have a " + function + " which is recommended since it has dynamic memory/resource allocation(s).";
584+
errmsg += type + " '$symbol' does not have a " + function + " which is recommended since it has dynamic memory/resource management.";
560585
}
561586

562587
return errmsg;

‎lib/token.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,9 @@ struct TokenImpl {
147147
TokenImpl() : mFunction(nullptr) {}
148148

149149
~TokenImpl();
150+
151+
TokenImpl(const TokenImpl &) = delete;
152+
TokenImpl operator=(const TokenImpl &) = delete;
150153
};
151154

152155
/// @addtogroup Core

‎test/testclass.cpp

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ class TestClass : public TestFixture {
5959
TEST_CASE(copyConstructor4); // base class with private constructor
6060
TEST_CASE(copyConstructor5); // multiple inheritance
6161
TEST_CASE(copyConstructor6); // array of pointers
62+
TEST_CASE(deletedMemberPointer); // deleted member pointer in destructor
6263
TEST_CASE(noOperatorEq); // class with memory management should have operator eq
6364
TEST_CASE(noDestructor); // class with memory management should have destructor
6465

@@ -893,7 +894,7 @@ class TestClass : public TestFixture {
893894
" ~F();\n"
894895
" F& operator=(const F&f);\n"
895896
"};");
896-
TODO_ASSERT_EQUALS("[test.cpp:8]: (warning) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s).\n", "", errout_str());
897+
TODO_ASSERT_EQUALS("[test.cpp:8]: (warning) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource management.\n", "", errout_str());
897898

898899
checkCopyConstructor("class F\n"
899900
"{\n"
@@ -951,7 +952,7 @@ class TestClass : public TestFixture {
951952
" ~F();\n"
952953
" F& operator=(const F&f);\n"
953954
"};");
954-
ASSERT_EQUALS("[test.cpp:5:7]: (warning) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s). [noCopyConstructor]\n", errout_str());
955+
ASSERT_EQUALS("[test.cpp:5:7]: (warning) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource management. [noCopyConstructor]\n", errout_str());
955956

956957
checkCopyConstructor("class F {\n"
957958
" char *p;\n"
@@ -970,7 +971,7 @@ class TestClass : public TestFixture {
970971
" ~F();\n"
971972
" F& operator=(const F&f);\n"
972973
"};");
973-
ASSERT_EQUALS("[test.cpp:3:10]: (warning) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s). [noCopyConstructor]\n", errout_str());
974+
ASSERT_EQUALS("[test.cpp:3:10]: (warning) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource management. [noCopyConstructor]\n", errout_str());
974975

975976
// #7198
976977
checkCopyConstructor("struct F {\n"
@@ -1074,21 +1075,48 @@ class TestClass : public TestFixture {
10741075
" }\n"
10751076
" char* a[5];\n"
10761077
"};\n");
1077-
TODO_ASSERT_EQUALS("[test.cpp:4]: (warning) Struct 'S' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s).\n"
1078-
"[test.cpp:4]: (warning) Struct 'S' does not have a operator= which is recommended since it has dynamic memory/resource allocation(s).\n"
1079-
"[test.cpp:4]: (warning) Struct 'S' does not have a destructor which is recommended since it has dynamic memory/resource allocation(s).\n",
1078+
TODO_ASSERT_EQUALS("[test.cpp:4]: (warning) Struct 'S' does not have a copy constructor which is recommended since it has dynamic memory/resource management.\n"
1079+
"[test.cpp:4]: (warning) Struct 'S' does not have a operator= which is recommended since it has dynamic memory/resource management.\n"
1080+
"[test.cpp:4]: (warning) Struct 'S' does not have a destructor which is recommended since it has dynamic memory/resource management.\n",
10801081
"",
10811082
errout_str());
10821083
}
10831084

1085+
void deletedMemberPointer() {
1086+
1087+
// delete ...
1088+
checkCopyConstructor("struct P {};\n"
1089+
"class C {\n"
1090+
" P *p;\n"
1091+
"public:\n"
1092+
" explicit C(P *p) : p(p) {}\n"
1093+
" ~C() { delete p; }\n"
1094+
" void f() {}\n"
1095+
"};\n");
1096+
ASSERT_EQUALS("[test.cpp:6:19]: (warning) Class 'C' does not have a copy constructor which is recommended since it has dynamic memory/resource management. [noCopyConstructor]\n"
1097+
"[test.cpp:6:19]: (warning) Class 'C' does not have a operator= which is recommended since it has dynamic memory/resource management. [noOperatorEq]\n", errout_str());
1098+
1099+
// free(...)
1100+
checkCopyConstructor("struct P {};\n"
1101+
"class C {\n"
1102+
" P *p;\n"
1103+
"public:\n"
1104+
" explicit C(P *p) : p(p) {}\n"
1105+
" ~C() { free(p); }\n"
1106+
" void f() {}\n"
1107+
"};\n");
1108+
ASSERT_EQUALS("[test.cpp:6:17]: (warning) Class 'C' does not have a copy constructor which is recommended since it has dynamic memory/resource management. [noCopyConstructor]\n"
1109+
"[test.cpp:6:17]: (warning) Class 'C' does not have a operator= which is recommended since it has dynamic memory/resource management. [noOperatorEq]\n", errout_str());
1110+
}
1111+
10841112
void noOperatorEq() {
10851113
checkCopyConstructor("struct F {\n"
10861114
" char* c;\n"
10871115
" F() { c = malloc(100); }\n"
10881116
" F(const F &f);\n"
10891117
" ~F();\n"
10901118
"};");
1091-
ASSERT_EQUALS("[test.cpp:3:10]: (warning) Struct 'F' does not have a operator= which is recommended since it has dynamic memory/resource allocation(s). [noOperatorEq]\n", errout_str());
1119+
ASSERT_EQUALS("[test.cpp:3:10]: (warning) Struct 'F' does not have a operator= which is recommended since it has dynamic memory/resource management. [noOperatorEq]\n", errout_str());
10921120

10931121
// defaulted operator=
10941122
checkCopyConstructor("struct F {\n"
@@ -1127,7 +1155,7 @@ class TestClass : public TestFixture {
11271155
" F(const F &f);\n"
11281156
" F&operator=(const F&);"
11291157
"};");
1130-
ASSERT_EQUALS("[test.cpp:3:10]: (warning) Struct 'F' does not have a destructor which is recommended since it has dynamic memory/resource allocation(s). [noDestructor]\n", errout_str());
1158+
ASSERT_EQUALS("[test.cpp:3:10]: (warning) Struct 'F' does not have a destructor which is recommended since it has dynamic memory/resource management. [noDestructor]\n", errout_str());
11311159

11321160
checkCopyConstructor("struct F {\n"
11331161
" C* c;\n"
@@ -1143,7 +1171,7 @@ class TestClass : public TestFixture {
11431171
" F(const F &f);\n"
11441172
" F& operator=(const F&);"
11451173
"};");
1146-
ASSERT_EQUALS("[test.cpp:3:10]: (warning) Struct 'F' does not have a destructor which is recommended since it has dynamic memory/resource allocation(s). [noDestructor]\n", errout_str());
1174+
ASSERT_EQUALS("[test.cpp:3:10]: (warning) Struct 'F' does not have a destructor which is recommended since it has dynamic memory/resource management. [noDestructor]\n", errout_str());
11471175

11481176
checkCopyConstructor("struct Data { int x; int y; };\n"
11491177
"struct F {\n"
@@ -1152,7 +1180,7 @@ class TestClass : public TestFixture {
11521180
" F(const F &f);\n"
11531181
" F&operator=(const F&);"
11541182
"};");
1155-
ASSERT_EQUALS("[test.cpp:4:10]: (warning) Struct 'F' does not have a destructor which is recommended since it has dynamic memory/resource allocation(s). [noDestructor]\n", errout_str());
1183+
ASSERT_EQUALS("[test.cpp:4:10]: (warning) Struct 'F' does not have a destructor which is recommended since it has dynamic memory/resource management. [noDestructor]\n", errout_str());
11561184

11571185
// defaulted destructor
11581186
checkCopyConstructor("struct F {\n"

0 commit comments

Comments
 (0)