Skip to content

fix #13915: Improve check: copy constructor and assignment operator should be defined when class deletes pointer #7703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danmar merged 5 commits into from
Jul 30, 2025
Merged

fix #13915: Improve check: copy constructor and assignment operator should be defined when class deletes pointer #7703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
56173f2
update error message
ludviggunne Jul 25, 2025
b6a1915
add test
ludviggunne Jul 25, 2025
85e6ad9
fix #13915
ludviggunne Jul 25, 2025
10929b7
add inline suppressions
ludviggunne Jul 25, 2025
6f36218
fixes
ludviggunne Jul 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 47 additions & 22 deletions lib/checkclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,30 +413,47 @@ void CheckClass::copyconstructors()

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

for (const Function &func : scope->functionList) {
if (func.type != FunctionType::eConstructor || !func.functionScope)
continue;
const Token* tok = func.token->linkAt(1);
for (const Token* const end = func.functionScope->bodyStart; tok != end; tok = tok->next()) {
if (Token::Match(tok, "%var% ( new") ||
(Token::Match(tok, "%var% ( %name% (") && mSettings->library.getAllocFuncInfo(tok->tokAt(2)))) {
const Variable* var = tok->variable();
if (var && var->isPointer() && var->scope() == scope)
allocatedVars[tok->varId()] = tok;
if (func.type == FunctionType::eConstructor && func.functionScope) {
// Allocations in constructors
const Token* tok = func.token->linkAt(1);
for (const Token* const end = func.functionScope->bodyStart; tok != end; tok = tok->next()) {
if (Token::Match(tok, "%var% ( new") ||
(Token::Match(tok, "%var% ( %name% (") && mSettings->library.getAllocFuncInfo(tok->tokAt(2)))) {
const Variable* var = tok->variable();
if (var && var->isPointer() && var->scope() == scope)
allocatedVars[tok->varId()] = tok;
}
}
}
for (const Token* const end = func.functionScope->bodyEnd; tok != end; tok = tok->next()) {
if (Token::Match(tok, "%var% = new") ||
(Token::Match(tok, "%var% = %name% (") && mSettings->library.getAllocFuncInfo(tok->tokAt(2)))) {
const Variable* var = tok->variable();
if (var && var->isPointer() && var->scope() == scope && !var->isStatic())
allocatedVars[tok->varId()] = tok;
for (const Token* const end = func.functionScope->bodyEnd; tok != end; tok = tok->next()) {
if (Token::Match(tok, "%var% = new") ||
(Token::Match(tok, "%var% = %name% (") && mSettings->library.getAllocFuncInfo(tok->tokAt(2)))) {
const Variable* var = tok->variable();
if (var && var->isPointer() && var->scope() == scope && !var->isStatic())
allocatedVars[tok->varId()] = tok;
}
}
} else if (func.type == FunctionType::eDestructor && func.functionScope) {
// Deallocations in destructors
const Token* tok = func.functionScope->bodyStart;
for (const Token* const end = func.functionScope->bodyEnd; tok != end; tok = tok->next()) {
if (Token::Match(tok, "delete %var%") ||
(Token::Match(tok, "%name% ( %var%") && mSettings->library.getDeallocFuncInfo(tok))) {
const Token *vartok = tok->str() == "delete" ? tok->next() : tok->tokAt(2);
const Variable* var = vartok->variable();
if (var && var->isPointer() && var->scope() == scope && !var->isStatic())
deallocatedVars[vartok->varId()] = vartok;
}
}
}
}

if (!allocatedVars.empty()) {
const bool hasAllocatedVars = !allocatedVars.empty();
const bool hasDeallocatedVars = !deallocatedVars.empty();

if (hasAllocatedVars || hasDeallocatedVars) {
const Function *funcCopyCtor = nullptr;
const Function *funcOperatorEq = nullptr;
const Function *funcDestructor = nullptr;
Expand All @@ -450,13 +467,21 @@ void CheckClass::copyconstructors()
}
if (!funcCopyCtor || funcCopyCtor->isDefault()) {
bool unknown = false;
if (!hasNonCopyableBase(scope, &unknown) && !unknown)
noCopyConstructorError(scope, funcCopyCtor, allocatedVars.cbegin()->second, unknown);
if (!hasNonCopyableBase(scope, &unknown) && !unknown) {
if (hasAllocatedVars)
noCopyConstructorError(scope, funcCopyCtor, allocatedVars.cbegin()->second, unknown);
else
noCopyConstructorError(scope, funcCopyCtor, deallocatedVars.cbegin()->second, unknown);
}
}
if (!funcOperatorEq || funcOperatorEq->isDefault()) {
bool unknown = false;
if (!hasNonCopyableBase(scope, &unknown) && !unknown)
noOperatorEqError(scope, funcOperatorEq, allocatedVars.cbegin()->second, unknown);
if (!hasNonCopyableBase(scope, &unknown) && !unknown) {
if (hasAllocatedVars)
noOperatorEqError(scope, funcOperatorEq, allocatedVars.cbegin()->second, unknown);
else
noOperatorEqError(scope, funcOperatorEq, deallocatedVars.cbegin()->second, unknown);
}
}
if (!funcDestructor || funcDestructor->isDefault()) {
const Token * mustDealloc = nullptr;
Expand Down Expand Up @@ -556,7 +581,7 @@ static std::string noMemberErrorMessage(const Scope *scope, const char function[
else
errmsg += " It is recommended to define or delete the " + std::string(function) + '.';
} else {
errmsg += type + " '$symbol' does not have a " + function + " which is recommended since it has dynamic memory/resource allocation(s).";
errmsg += type + " '$symbol' does not have a " + function + " which is recommended since it has dynamic memory/resource management.";
}

return errmsg;
Expand Down
3 changes: 3 additions & 0 deletions lib/token.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,9 @@ struct TokenImpl {
TokenImpl() : mFunction(nullptr) {}

~TokenImpl();

TokenImpl(const TokenImpl &) = delete;
TokenImpl operator=(const TokenImpl &) = delete;
};

/// @addtogroup Core
Expand Down
48 changes: 38 additions & 10 deletions test/testclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class TestClass : public TestFixture {
TEST_CASE(copyConstructor4); // base class with private constructor
TEST_CASE(copyConstructor5); // multiple inheritance
TEST_CASE(copyConstructor6); // array of pointers
TEST_CASE(deletedMemberPointer); // deleted member pointer in destructor
TEST_CASE(noOperatorEq); // class with memory management should have operator eq
TEST_CASE(noDestructor); // class with memory management should have destructor

Expand Down Expand Up @@ -893,7 +894,7 @@ class TestClass : public TestFixture {
" ~F();\n"
" F& operator=(const F&f);\n"
"};");
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());
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());

checkCopyConstructor("class F\n"
"{\n"
Expand Down Expand Up @@ -951,7 +952,7 @@ class TestClass : public TestFixture {
" ~F();\n"
" F& operator=(const F&f);\n"
"};");
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());
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());

checkCopyConstructor("class F {\n"
" char *p;\n"
Expand All @@ -970,7 +971,7 @@ class TestClass : public TestFixture {
" ~F();\n"
" F& operator=(const F&f);\n"
"};");
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());
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());

// #7198
checkCopyConstructor("struct F {\n"
Expand Down Expand Up @@ -1074,21 +1075,48 @@ class TestClass : public TestFixture {
" }\n"
" char* a[5];\n"
"};\n");
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"
"[test.cpp:4]: (warning) Struct 'S' does not have a operator= which is recommended since it has dynamic memory/resource allocation(s).\n"
"[test.cpp:4]: (warning) Struct 'S' does not have a destructor which is recommended since it has dynamic memory/resource allocation(s).\n",
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"
"[test.cpp:4]: (warning) Struct 'S' does not have a operator= which is recommended since it has dynamic memory/resource management.\n"
"[test.cpp:4]: (warning) Struct 'S' does not have a destructor which is recommended since it has dynamic memory/resource management.\n",
"",
errout_str());
}

void deletedMemberPointer() {

// delete ...
checkCopyConstructor("struct P {};\n"
"class C {\n"
" P *p;\n"
"public:\n"
" explicit C(P *p) : p(p) {}\n"
" ~C() { delete p; }\n"
" void f() {}\n"
"};\n");
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"
"[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());

// free(...)
checkCopyConstructor("struct P {};\n"
"class C {\n"
" P *p;\n"
"public:\n"
" explicit C(P *p) : p(p) {}\n"
" ~C() { free(p); }\n"
" void f() {}\n"
"};\n");
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"
"[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());
}

void noOperatorEq() {
checkCopyConstructor("struct F {\n"
" char* c;\n"
" F() { c = malloc(100); }\n"
" F(const F &f);\n"
" ~F();\n"
"};");
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());
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());

// defaulted operator=
checkCopyConstructor("struct F {\n"
Expand Down Expand Up @@ -1127,7 +1155,7 @@ class TestClass : public TestFixture {
" F(const F &f);\n"
" F&operator=(const F&);"
"};");
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());
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());

checkCopyConstructor("struct F {\n"
" C* c;\n"
Expand All @@ -1143,7 +1171,7 @@ class TestClass : public TestFixture {
" F(const F &f);\n"
" F& operator=(const F&);"
"};");
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());
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());

checkCopyConstructor("struct Data { int x; int y; };\n"
"struct F {\n"
Expand All @@ -1152,7 +1180,7 @@ class TestClass : public TestFixture {
" F(const F &f);\n"
" F&operator=(const F&);"
"};");
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());
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());

// defaulted destructor
checkCopyConstructor("struct F {\n"
Expand Down