From 40b6f6b3dd3231636ffd7479e01561e5a22731ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 1 May 2018 15:31:13 +0200 Subject: [PATCH] CheckClass: Fix the noDestructor warning --- lib/checkclass.cpp | 43 ++++++++++++++++++++++++++++--------------- lib/checkclass.h | 12 ++++++------ test/testclass.cpp | 30 ++++++++++++++++++++++++------ 3 files changed, 58 insertions(+), 27 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 68643ef3f..bb02c9dd3 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -341,12 +341,27 @@ void CheckClass::copyconstructors() if (!hasCopyCtor) { bool unknown = false; if (!isNonCopyable(scope, &unknown)) - noCopyConstructorError(scope, unknown); + noCopyConstructorError(scope, allocatedVars.begin()->second, unknown); } if (!hasOperatorEq) - noOperatorEqError(scope); + noOperatorEqError(scope, allocatedVars.begin()->second); if (!hasDestructor) { - // TODO: how are pointers allocated? noDestructorError(scope); + const Token * mustDealloc = nullptr; + for (std::map::const_iterator it = allocatedVars.begin(); it != allocatedVars.end(); ++it) { + if (!Token::Match(it->second, "%var% [(=] new %type%")) { + mustDealloc = it->second; + break; + } + if (it->second->valueType() && it->second->valueType()->isIntegral()) { + mustDealloc = it->second; + break; + } + const Variable *var = it->second->variable(); + if (var && var->typeScope() && var->typeScope()->functionList.empty() && var->type()->derivedFrom.empty()) + mustDealloc = it->second; + } + if (mustDealloc) + noDestructorError(scope, mustDealloc); } } @@ -412,34 +427,32 @@ void CheckClass::copyConstructorShallowCopyError(const Token *tok, const std::st "$symbol:" + varname + "\nValue of pointer '$symbol', which points to allocated memory, is copied in copy constructor instead of allocating new memory.", CWE398, false); } -void CheckClass::noCopyConstructorError(const Scope *scope, bool inconclusive) +void CheckClass::noCopyConstructorError(const Scope *scope, const Token *alloc, bool inconclusive) { - const Token *tok = scope ? scope->classDef : nullptr; const std::string &classname = scope ? scope->className : "class"; bool isStruct = scope ? (scope->type == Scope::eStruct) : false; - reportError(tok, Severity::style, "noCopyConstructor", + reportError(alloc, Severity::style, "noCopyConstructor", "$symbol:" + classname + "\n" + - std::string(isStruct ? "struct" : "class") + " '$symbol' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.", CWE398, inconclusive); + std::string(isStruct ? "Struct" : "Class") + " '$symbol' does not have a copy constructor but it has dynamic memory/resource allocation. It is recommended to delete or define the copy constructor.", CWE398, inconclusive); } -void CheckClass::noOperatorEqError(const Scope *scope) +void CheckClass::noOperatorEqError(const Scope *scope, const Token *alloc) { - const Token *tok = scope ? scope->classDef : nullptr; const std::string &classname = scope ? scope->className : "class"; bool isStruct = scope ? (scope->type == Scope::eStruct) : false; - reportError(tok, Severity::style, "noOperatorEq", + reportError(alloc, Severity::style, "noOperatorEq", "$symbol:" + classname + "\n" + - std::string(isStruct ? "struct" : "class") + " '$symbol' does not have a assignment operator which is recommended since the class contains a pointer to allocated memory.", CWE398, false); + std::string(isStruct ? "Struct" : "Class") + " '$symbol' does not have a assignment operator but it has dynamic memory/resource allocation. It is recommended to delete or define the assignment operator.", CWE398, false); } -void CheckClass::noDestructorError(const Scope *scope) +void CheckClass::noDestructorError(const Scope *scope, const Token *alloc) { - const Token *tok = scope ? scope->classDef : nullptr; const std::string &classname = scope ? scope->className : "class"; bool isStruct = scope ? (scope->type == Scope::eStruct) : false; - reportError(tok, Severity::style, "noDestructor", + + reportError(alloc, Severity::style, "noDestructor", "$symbol:" + classname + "\n" + - std::string(isStruct ? "struct" : "class") + " '$symbol' does not have a destructor which is recommended since the class contains a pointer to allocated memory.", CWE398, false); + std::string(isStruct ? "Struct" : "Class") + " '$symbol' does not have a destructor which is recommended since the class has dynamic memory/resource allocation.", CWE398, false); } bool CheckClass::canNotCopy(const Scope *scope) diff --git a/lib/checkclass.h b/lib/checkclass.h index a7977f0a9..cb6525297 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -168,9 +168,9 @@ private: void noExplicitConstructorError(const Token *tok, const std::string &classname, bool isStruct); //void copyConstructorMallocError(const Token *cctor, const Token *alloc, const std::string& var_name); void copyConstructorShallowCopyError(const Token *tok, const std::string& varname); - void noCopyConstructorError(const Scope *scope, bool inconclusive); - void noOperatorEqError(const Scope *scope); - void noDestructorError(const Scope *scope); + void noCopyConstructorError(const Scope *scope, const Token *alloc, bool inconclusive); + void noOperatorEqError(const Scope *scope, const Token *alloc); + void noDestructorError(const Scope *scope, const Token *alloc); void uninitVarError(const Token *tok, bool isprivate, const std::string &classname, const std::string &varname, bool inconclusive); void operatorEqVarError(const Token *tok, const std::string &classname, const std::string &varname, bool inconclusive); void unusedPrivateFunctionError(const Token *tok, const std::string &classname, const std::string &funcname); @@ -204,9 +204,9 @@ private: c.noExplicitConstructorError(nullptr, "classname", false); //c.copyConstructorMallocError(nullptr, 0, "var"); c.copyConstructorShallowCopyError(nullptr, "var"); - c.noCopyConstructorError(nullptr, false); - c.noOperatorEqError(nullptr); - c.noDestructorError(nullptr); + c.noCopyConstructorError(nullptr, nullptr, false); + c.noOperatorEqError(nullptr, nullptr); + c.noDestructorError(nullptr, nullptr); c.uninitVarError(nullptr, false, "classname", "varname", false); c.operatorEqVarError(nullptr, "classname", emptyString, false); c.unusedPrivateFunctionError(nullptr, "classname", "funcname"); diff --git a/test/testclass.cpp b/test/testclass.cpp index 4b2b2ba76..41fc17708 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -700,7 +700,7 @@ private: " ~F();\n" " F& operator=(const F&f);\n" "};"); - ASSERT_EQUALS("[test.cpp:1]: (style) class 'F' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8]: (style) Class '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()); checkCopyConstructor("class F\n" "{\n" @@ -735,7 +735,7 @@ private: " ~F();\n" " F& operator=(const F&f);\n" "};"); - ASSERT_EQUALS("[test.cpp:1]: (style, inconclusive) class 'F' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (style, inconclusive) Class '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()); checkCopyConstructor("class E { E(E&); };\n" // non-copyable "class F : E\n" @@ -758,7 +758,7 @@ private: " ~F();\n" " F& operator=(const F&f);\n" "};"); - ASSERT_EQUALS("[test.cpp:2]: (style) class 'F' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (style) Class '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()); checkCopyConstructor("class F {\n" " char *p;\n" @@ -777,7 +777,7 @@ private: " ~F();\n" " F& operator=(const F&f);\n" "};"); - ASSERT_EQUALS("[test.cpp:1]: (style) class 'F' does not have a copy constructor which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Class '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()); // #7198 checkCopyConstructor("struct F {\n" @@ -814,7 +814,7 @@ private: " F(const F &f);\n" " ~F();\n" "};"); - ASSERT_EQUALS("[test.cpp:1]: (style) struct 'F' does not have a assignment operator which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); + 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()); } void noDestructor() { @@ -824,7 +824,25 @@ private: " F(const F &f);\n" " F&operator=(const F&);" "};"); - // TODO ASSERT_EQUALS("[test.cpp:1]: (style) struct 'F' does not have a destructor which is recommended since the class contains a pointer to allocated memory.\n", errout.str()); + 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()); + + checkCopyConstructor("struct F {\n" + " C* c;\n" + " F() { c = new C; }\n" + " F(const F &f);\n" + " F&operator=(const F&);" + "};"); + ASSERT_EQUALS("", errout.str()); + + + checkCopyConstructor("struct Data { int x; int y; };\n" + "struct F {\n" + " Data* c;\n" + " F() { c = new Data; }\n" + " F(const F &f);\n" + " 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()); } // Check the operator Equal