diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index df2170050ce..d5621371083 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -413,30 +413,47 @@ void CheckClass::copyconstructors() for (const Scope * scope : mSymbolDatabase->classAndStructScopes) { std::map allocatedVars; + std::map 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; @@ -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; @@ -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; diff --git a/lib/token.h b/lib/token.h index ecdba24007b..b2c433af164 100644 --- a/lib/token.h +++ b/lib/token.h @@ -147,6 +147,9 @@ struct TokenImpl { TokenImpl() : mFunction(nullptr) {} ~TokenImpl(); + + TokenImpl(const TokenImpl &) = delete; + TokenImpl operator=(const TokenImpl &) = delete; }; /// @addtogroup Core diff --git a/test/testclass.cpp b/test/testclass.cpp index e0a77044b3e..2cb62d257f2 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -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 @@ -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" @@ -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" @@ -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" @@ -1074,13 +1075,40 @@ 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" @@ -1088,7 +1116,7 @@ class TestClass : public TestFixture { " 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" @@ -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" @@ -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" @@ -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"