From 99003c20844574e13191da003f2a995010a43946 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Fri, 4 May 2018 14:58:38 +0200 Subject: [PATCH] CheckClass: Better handling of defaulted and deleted functions in the noCopyConstructor/noOperatorEq/noDestructor --- lib/checkclass.cpp | 18 ++++++------- test/testclass.cpp | 63 ++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 11 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 5b4854d38..7996a92c1 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -327,25 +327,25 @@ void CheckClass::copyconstructors() } if (!allocatedVars.empty()) { - bool hasCopyCtor = false; - bool hasOperatorEq = false; - bool hasDestructor = false; + const Function *funcCopyCtor = nullptr; + const Function *funcOperatorEq = nullptr; + const Function *funcDestructor = nullptr; for (const Function &func : scope->functionList) { if (func.type == Function::eCopyConstructor) - hasCopyCtor = true; + funcCopyCtor = &func; else if (func.type == Function::eOperatorEqual) - hasOperatorEq = true; + funcOperatorEq = &func; else if (func.type == Function::eDestructor) - hasDestructor = true; + funcDestructor = &func; } - if (!hasCopyCtor) { + if (!funcCopyCtor || funcCopyCtor->isDefault()) { bool unknown = false; if (!isNonCopyable(scope, &unknown)) noCopyConstructorError(scope, allocatedVars.begin()->second, unknown); } - if (!hasOperatorEq) + if (!funcOperatorEq || funcOperatorEq->isDefault()) noOperatorEqError(scope, allocatedVars.begin()->second); - if (!hasDestructor) { + if (!funcDestructor || funcDestructor->isDefault()) { const Token * mustDealloc = nullptr; for (std::map::const_iterator it = allocatedVars.begin(); it != allocatedVars.end(); ++it) { if (!Token::Match(it->second, "%var% [(=] new %type%")) { diff --git a/test/testclass.cpp b/test/testclass.cpp index 41fc17708..767fc01b1 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -68,6 +68,7 @@ private: TEST_CASE(copyConstructor1); TEST_CASE(copyConstructor2); // ticket #4458 + TEST_CASE(copyConstructor3); // defaulted/deleted TEST_CASE(noOperatorEq); // class with memory management should have operator eq TEST_CASE(noDestructor); // class with memory management should have destructor @@ -789,7 +790,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void copyConstructor2() { // ticket #4458 checkCopyConstructor("template \n" "class Vector\n" @@ -807,6 +807,26 @@ private: ASSERT_EQUALS("", errout.str()); } + void copyConstructor3() { + checkCopyConstructor("struct F {\n" + " char* c;\n" + " F() { c = malloc(100); }\n" + " F(const F &f) = delete;\n" + " F&operator=(const F &f);\n" + " ~F();\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkCopyConstructor("struct F {\n" + " char* c;\n" + " F() { c = malloc(100); }\n" + " F(const F &f) = default;\n" + " F&operator=(const F &f);\n" + " ~F();\n" + "};"); + ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' does not have a copy constructor but it has dynamic memory/resource allocation. It is recommended to delete or define the copy constructor.\n", errout.str()); + } + void noOperatorEq() { checkCopyConstructor("struct F {\n" " char* c;\n" @@ -815,6 +835,26 @@ private: " ~F();\n" "};"); ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' does not have a assignment operator but it has dynamic memory/resource allocation. It is recommended to delete or define the assignment operator.\n", errout.str()); + + // defaulted operator= + checkCopyConstructor("struct F {\n" + " char* c;\n" + " F() { c = malloc(100); }\n" + " F(const F &f);\n" + " F &operator=(const F &f) = default;\n" + " ~F();\n" + "};"); + ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' does not have a assignment operator but it has dynamic memory/resource allocation. It is recommended to delete or define the assignment operator.\n", errout.str()); + + // deleted operator= + checkCopyConstructor("struct F {\n" + " char* c;\n" + " F() { c = malloc(100); }\n" + " F(const F &f);\n" + " F &operator=(const F &f) = delete;\n" + " ~F();\n" + "};"); + ASSERT_EQUALS("", errout.str()); } void noDestructor() { @@ -834,7 +874,6 @@ private: "};"); ASSERT_EQUALS("", errout.str()); - checkCopyConstructor("struct Data { int x; int y; };\n" "struct F {\n" " Data* c;\n" @@ -843,6 +882,26 @@ private: " F&operator=(const F&);" "};"); ASSERT_EQUALS("[test.cpp:4]: (style) Struct 'F' does not have a destructor which is recommended since the class has dynamic memory/resource allocation.\n", errout.str()); + + // defaulted destructor + checkCopyConstructor("struct F {\n" + " char* c;\n" + " F() { c = malloc(100); }\n" + " F(const F &f);\n" + " F &operator=(const F &f);\n" + " ~F() = default;\n" + "};"); + ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' does not have a destructor which is recommended since the class has dynamic memory/resource allocation.\n", errout.str()); + + // deleted destructor + checkCopyConstructor("struct F {\n" + " char* c;\n" + " F() { c = malloc(100); }\n" + " F(const F &f);\n" + " F &operator=(const F &f);\n" + " ~F() = delete;\n" + "};"); + ASSERT_EQUALS("", errout.str()); } // Check the operator Equal