diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 7996a92c1..1c9122458 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -341,10 +341,10 @@ void CheckClass::copyconstructors() if (!funcCopyCtor || funcCopyCtor->isDefault()) { bool unknown = false; if (!isNonCopyable(scope, &unknown)) - noCopyConstructorError(scope, allocatedVars.begin()->second, unknown); + noCopyConstructorError(scope, funcCopyCtor, allocatedVars.begin()->second, unknown); } if (!funcOperatorEq || funcOperatorEq->isDefault()) - noOperatorEqError(scope, allocatedVars.begin()->second); + noOperatorEqError(scope, funcOperatorEq, allocatedVars.begin()->second); if (!funcDestructor || funcDestructor->isDefault()) { const Token * mustDealloc = nullptr; for (std::map::const_iterator it = allocatedVars.begin(); it != allocatedVars.end(); ++it) { @@ -361,7 +361,7 @@ void CheckClass::copyconstructors() mustDealloc = it->second; } if (mustDealloc) - noDestructorError(scope, mustDealloc); + noDestructorError(scope, funcDestructor, mustDealloc); } } @@ -427,32 +427,39 @@ 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, const Token *alloc, bool inconclusive) +static std::string noMemberErrorMessage(const Scope *scope, const char function[], bool isdefault) { const std::string &classname = scope ? scope->className : "class"; - bool isStruct = scope ? (scope->type == Scope::eStruct) : false; - reportError(alloc, Severity::style, "noCopyConstructor", - "$symbol:" + classname + "\n" + - 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); + const std::string type = (scope && scope->type == Scope::eStruct) ? "Struct" : "Class"; + bool isDestructor = (function[0] == 'd'); + std::string errmsg = "$symbol:" + classname + '\n'; + + if (isdefault) { + errmsg += type + " '$symbol' has dynamic memory/resource allocation(s). The " + function + " is explicitly defaulted but the default " + function + " does not work well."; + if (isDestructor) + errmsg += " It is recommended to define the " + std::string(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)."; + } + + return errmsg; } -void CheckClass::noOperatorEqError(const Scope *scope, const Token *alloc) +void CheckClass::noCopyConstructorError(const Scope *scope, bool isdefault, const Token *alloc, bool inconclusive) { - const std::string &classname = scope ? scope->className : "class"; - bool isStruct = scope ? (scope->type == Scope::eStruct) : false; - reportError(alloc, Severity::style, "noOperatorEq", - "$symbol:" + classname + "\n" + - 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); + reportError(alloc, Severity::style, "noCopyConstructor", noMemberErrorMessage(scope, "copy constructor", isdefault), CWE398, inconclusive); } -void CheckClass::noDestructorError(const Scope *scope, const Token *alloc) +void CheckClass::noOperatorEqError(const Scope *scope, bool isdefault, const Token *alloc) { - const std::string &classname = scope ? scope->className : "class"; - bool isStruct = scope ? (scope->type == Scope::eStruct) : false; + reportError(alloc, Severity::style, "noOperatorEq", noMemberErrorMessage(scope, "operator=", isdefault), CWE398, false); +} - 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 has dynamic memory/resource allocation.", CWE398, false); +void CheckClass::noDestructorError(const Scope *scope, bool isdefault, const Token *alloc) +{ + reportError(alloc, Severity::style, "noDestructor", noMemberErrorMessage(scope, "destructor", isdefault), CWE398, false); } bool CheckClass::canNotCopy(const Scope *scope) diff --git a/lib/checkclass.h b/lib/checkclass.h index cb6525297..079bf3896 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, const Token *alloc, bool inconclusive); - void noOperatorEqError(const Scope *scope, const Token *alloc); - void noDestructorError(const Scope *scope, const Token *alloc); + void noCopyConstructorError(const Scope *scope, bool isdefault, const Token *alloc, bool inconclusive); + void noOperatorEqError(const Scope *scope, bool isdefault, const Token *alloc); + void noDestructorError(const Scope *scope, bool isdefault, 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, nullptr, false); - c.noOperatorEqError(nullptr, nullptr); - c.noDestructorError(nullptr, nullptr); + c.noCopyConstructorError(nullptr, false, nullptr, false); + c.noOperatorEqError(nullptr, false, nullptr); + c.noDestructorError(nullptr, false, 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 767fc01b1..84e3ca5a3 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -701,7 +701,7 @@ private: " ~F();\n" " F& operator=(const F&f);\n" "};"); - 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()); + ASSERT_EQUALS("[test.cpp:8]: (style) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s).\n", errout.str()); checkCopyConstructor("class F\n" "{\n" @@ -736,7 +736,7 @@ private: " ~F();\n" " F& operator=(const F&f);\n" "};"); - 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()); + ASSERT_EQUALS("[test.cpp:5]: (style, inconclusive) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s).\n", errout.str()); checkCopyConstructor("class E { E(E&); };\n" // non-copyable "class F : E\n" @@ -759,7 +759,7 @@ private: " ~F();\n" " F& operator=(const F&f);\n" "};"); - 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()); + ASSERT_EQUALS("[test.cpp:5]: (style) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s).\n", errout.str()); checkCopyConstructor("class F {\n" " char *p;\n" @@ -778,7 +778,7 @@ private: " ~F();\n" " F& operator=(const F&f);\n" "};"); - 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()); + ASSERT_EQUALS("[test.cpp:3]: (style) Class 'F' does not have a copy constructor which is recommended since it has dynamic memory/resource allocation(s).\n", errout.str()); // #7198 checkCopyConstructor("struct F {\n" @@ -824,7 +824,7 @@ private: " 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()); + ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' has dynamic memory/resource allocation(s). The copy constructor is explicitly defaulted but the default copy constructor does not work well. It is recommended to define or delete the copy constructor.\n", errout.str()); } void noOperatorEq() { @@ -834,7 +834,7 @@ private: " F(const F &f);\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()); + ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' does not have a operator= which is recommended since it has dynamic memory/resource allocation(s).\n", errout.str()); // defaulted operator= checkCopyConstructor("struct F {\n" @@ -844,7 +844,7 @@ private: " 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()); + ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' has dynamic memory/resource allocation(s). The operator= is explicitly defaulted but the default operator= does not work well. It is recommended to define or delete the operator=.\n", errout.str()); // deleted operator= checkCopyConstructor("struct F {\n" @@ -864,7 +864,7 @@ private: " F(const F &f);\n" " F&operator=(const F&);" "};"); - 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()); + ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' does not have a destructor which is recommended since it has dynamic memory/resource allocation(s).\n", errout.str()); checkCopyConstructor("struct F {\n" " C* c;\n" @@ -881,7 +881,7 @@ private: " 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()); + ASSERT_EQUALS("[test.cpp:4]: (style) Struct 'F' does not have a destructor which is recommended since it has dynamic memory/resource allocation(s).\n", errout.str()); // defaulted destructor checkCopyConstructor("struct F {\n" @@ -891,7 +891,7 @@ private: " 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()); + ASSERT_EQUALS("[test.cpp:3]: (style) Struct 'F' has dynamic memory/resource allocation(s). The destructor is explicitly defaulted but the default destructor does not work well. It is recommended to define the destructor.\n", errout.str()); // deleted destructor checkCopyConstructor("struct F {\n"