From 6fb25dcaa4624b5ccc0017f1e4376665f9de6470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 24 Apr 2018 16:07:58 +0200 Subject: [PATCH] CheckClass: Changed checker for 'copy constructor' and 'operator=' to a 'rule of 3' checker --- lib/checkclass.cpp | 97 ++++++++++++++++++++++------------ lib/checkclass.h | 12 ++--- test/testclass.cpp | 127 +++++++++++++++++++++------------------------ 3 files changed, 130 insertions(+), 106 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index cc87cf49e..4af6a2b63 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2368,17 +2368,11 @@ void CheckClass::duplInheritedMembersError(const Token *tok1, const Token* tok2, } -//--------------------------------------------------------------------------- -// Check that copy constructor and operator defined together -//--------------------------------------------------------------------------- +//--------------------------------------------------------------------------------------------------------------------- +// Rule of 3: If a class defines a 'copy constructor', 'destructor' or 'operator='; then all these must be defined +//--------------------------------------------------------------------------------------------------------------------- -enum CtorType { - NO, - WITHOUT_BODY, - WITH_BODY -}; - -void CheckClass::checkCopyCtorAndEqOperator() +void CheckClass::checkRuleOf3() { if (!_settings->isEnabled(Settings::WARNING)) return; @@ -2386,8 +2380,8 @@ void CheckClass::checkCopyCtorAndEqOperator() for (const Scope * scope : symbolDatabase->classAndStructScopes) { bool hasNonStaticVars = false; - for (std::list::const_iterator var = scope->varlist.begin(); var != scope->varlist.end(); ++var) { - if (!var->isStatic()) { + for (const Variable &var : scope->varlist) { + if (!var.isStatic()) { hasNonStaticVars = true; break; } @@ -2395,22 +2389,31 @@ void CheckClass::checkCopyCtorAndEqOperator() if (!hasNonStaticVars) continue; - CtorType copyCtors = CtorType::NO; - bool moveCtor = false; - CtorType assignmentOperators = CtorType::NO; + enum CtorType { + NO, + WITHOUT_BODY, + WITH_BODY + }; - std::list::const_iterator func; - for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - if (copyCtors == CtorType::NO && func->type == Function::eCopyConstructor) { - copyCtors = func->hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY; + CtorType copyCtor = CtorType::NO; + CtorType assignmentOperator = CtorType::NO; + bool destructor = false; + bool moveCtor = false; + + for (const Function &func : scope->functionList) { + if (copyCtor != CtorType::WITH_BODY && func.type == Function::eCopyConstructor) { + copyCtor = func.hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY; } - if (assignmentOperators == CtorType::NO && func->type == Function::eOperatorEqual) { - const Variable * variable = func->getArgumentVar(0); + if (assignmentOperator != CtorType::WITH_BODY && func.type == Function::eOperatorEqual) { + const Variable * variable = func.getArgumentVar(0); if (variable && variable->type() && variable->type()->classScope == scope) { - assignmentOperators = func->hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY; + assignmentOperator = func.hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY; } } - if (func->type == Function::eMoveConstructor) { + if (func.type == Function::eDestructor) { + destructor = true; + } + if (func.type == Function::eMoveConstructor) { moveCtor = true; break; } @@ -2419,20 +2422,48 @@ void CheckClass::checkCopyCtorAndEqOperator() if (moveCtor) continue; - if ((copyCtors == CtorType::WITH_BODY && assignmentOperators == CtorType::NO) || - (copyCtors == CtorType::NO && assignmentOperators == CtorType::WITH_BODY)) - copyCtorAndEqOperatorError(scope->classDef, scope->className, scope->type == Scope::eStruct, copyCtors == CtorType::WITH_BODY); + // No method defined + if (copyCtor != CtorType::WITH_BODY && assignmentOperator != CtorType::WITH_BODY && !destructor) + continue; + + // all 3 methods are defined + if (copyCtor == CtorType::WITH_BODY && assignmentOperator == CtorType::WITH_BODY && destructor) + continue; + + ruleOf3Error(scope->classDef, scope->className, scope->type == Scope::eStruct, copyCtor == CtorType::WITH_BODY, assignmentOperator == CtorType::WITH_BODY, destructor); } } -void CheckClass::copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor) +void CheckClass::ruleOf3Error(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor, bool hasAssignmentOperator, bool hasDestructor) { - const std::string message = "$symbol:" + classname + "\n" - "The " + std::string(isStruct ? "struct" : "class") + " '$symbol' has '" + - getFunctionTypeName(hasCopyCtor ? Function::eCopyConstructor : Function::eOperatorEqual) + - "' but lack of '" + getFunctionTypeName(hasCopyCtor ? Function::eOperatorEqual : Function::eCopyConstructor) + - "'."; - reportError(tok, Severity::warning, "copyCtorAndEqOperator", message); + std::string message = "$symbol:" + classname + "\n" + "The " + std::string(isStruct ? "struct" : "class") + " '$symbol' defines"; + if (hasCopyCtor) + message += " '" + std::string(getFunctionTypeName(Function::eCopyConstructor)) + '\''; + if (hasAssignmentOperator) { + if (hasCopyCtor) + message += " and"; + message += " '" + std::string(getFunctionTypeName(Function::eOperatorEqual)) + '\''; + } + if (hasDestructor) { + if (hasCopyCtor || hasAssignmentOperator) + message += " and"; + message += " '" + std::string(getFunctionTypeName(Function::eDestructor)) + '\''; + } + message += " but does not define"; + if (!hasCopyCtor) + message += " '" + std::string(getFunctionTypeName(Function::eCopyConstructor)) + '\''; + if (!hasAssignmentOperator) { + if (!hasCopyCtor) + message += " and"; + message += " '" + std::string(getFunctionTypeName(Function::eOperatorEqual)) + '\''; + } + if (!hasDestructor) { + if (!hasCopyCtor || !hasAssignmentOperator) + message += " and"; + message += " '" + std::string(getFunctionTypeName(Function::eDestructor)) + '\''; + } + reportError(tok, Severity::warning, "ruleOf3", message); } void CheckClass::checkUnsafeClassDivZero(bool test) diff --git a/lib/checkclass.h b/lib/checkclass.h index af85f7a3f..d97b3ef93 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -89,7 +89,7 @@ public: checkClass.checkDuplInheritedMembers(); checkClass.checkExplicitConstructors(); - checkClass.checkCopyCtorAndEqOperator(); + checkClass.checkRuleOf3(); } @@ -149,8 +149,8 @@ public: /** @brief Check duplicated inherited members */ void checkDuplInheritedMembers(); - /** @brief Check that copy constructor and operator defined together */ - void checkCopyCtorAndEqOperator(); + /** @brief Rule of 3: If a class defines a 'copy constructor', 'destructor' or 'operator='; then all these must be defined */ + void checkRuleOf3(); /** @brief Check that arbitrary usage of the public interface does not result in division by zero */ void checkUnsafeClassDivZero(bool test=false); @@ -187,7 +187,7 @@ private: void pureVirtualFunctionCallInConstructorError(const Function * scopeFunction, const std::list & tokStack, const std::string &purefuncname); void virtualFunctionCallInConstructorError(const Function * scopeFunction, const std::list & tokStack, const std::string &funcname); void duplInheritedMembersError(const Token* tok1, const Token* tok2, const std::string &derivedname, const std::string &basename, const std::string &variablename, bool derivedIsStruct, bool baseIsStruct); - void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor); + void ruleOf3Error(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor, bool hasAssignmentOperator, bool hasDestructor); void unsafeClassDivZeroError(const Token *tok, const std::string &className, const std::string &methodName, const std::string &varName); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { @@ -218,7 +218,7 @@ private: c.suggestInitializationList(nullptr, "variable"); c.selfInitializationError(nullptr, "var"); c.duplInheritedMembersError(nullptr, nullptr, "class", "class", "variable", false, false); - c.copyCtorAndEqOperatorError(nullptr, "class", false, false); + c.ruleOf3Error(nullptr, "class", false, false, true, true); c.unsafeClassDivZeroError(nullptr, "Class", "dostuff", "x"); c.pureVirtualFunctionCallInConstructorError(nullptr, std::list(), "f"); c.virtualFunctionCallInConstructorError(nullptr, std::list(), "f"); @@ -248,7 +248,7 @@ private: "- Suspicious subtraction from 'this'\n" "- Call of pure virtual function in constructor/destructor\n" "- Duplicated inherited data members\n" - "- If 'copy constructor' defined, 'operator=' also should be defined and vice versa\n" + "- Rule of 3: If a class defines a 'copy constructor', 'destructor' or 'operator='; then all these must be defined\n" "- Check that arbitrary usage of public interface does not result in division by zero\n"; } diff --git a/test/testclass.cpp b/test/testclass.cpp index 21225030e..e5489ebc4 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -188,12 +188,12 @@ private: TEST_CASE(duplInheritedMembers); TEST_CASE(explicitConstructors); - TEST_CASE(copyCtorAndEqOperator); + TEST_CASE(ruleOf3); TEST_CASE(unsafeClassDivZero); } - void checkCopyCtorAndEqOperator(const char code[]) { + void checkRuleOf3(const char code[]) { // Clear the error log errout.str(""); Settings settings; @@ -207,93 +207,86 @@ private: // Check.. CheckClass checkClass(&tokenizer, &settings, this); - checkClass.checkCopyCtorAndEqOperator(); + checkClass.checkRuleOf3(); } - void copyCtorAndEqOperator() { - checkCopyCtorAndEqOperator("class A \n" - "{ \n" - " A(const A& other) { } \n" - " A& operator=(const A& other) { return *this; }\n" - "};"); + void ruleOf3() { + checkRuleOf3("class A {\n" + " A(const A& other) { } \n" + " A& operator=(const A& other) { return *this; }\n" + "};"); ASSERT_EQUALS("", errout.str()); - checkCopyCtorAndEqOperator("class A \n" - "{ \n" - "};"); + checkRuleOf3("class A {};"); ASSERT_EQUALS("", errout.str()); - checkCopyCtorAndEqOperator("class A \n" - "{ \n" - " A(const A& other) { } \n" - "};"); + checkRuleOf3("class A {\n" + " A(const A& other) { } \n" + "};"); ASSERT_EQUALS("", errout.str()); - checkCopyCtorAndEqOperator("class A \n" - "{ \n" - " A& operator=(const A& other) { return *this; }\n" - "};"); + checkRuleOf3("class A {\n" + " A& operator=(const A& other) { return *this; }\n" + "};"); ASSERT_EQUALS("", errout.str()); - checkCopyCtorAndEqOperator("class A \n" - "{ \n" - " A(const A& other) { } \n" - " int x;\n" - "};"); - ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' has 'copy constructor' but lack of 'operator='.\n", errout.str()); + checkRuleOf3("class A {\n" + " A(const A& other) { } \n" + " int x;\n" + "};"); + ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' defines 'copy constructor' but does not define 'operator=' and 'destructor'\n", errout.str()); - checkCopyCtorAndEqOperator("class A \n" - "{ \n" - " A& operator=(const A& other) { return *this; }\n" - " int x;\n" - "};"); - ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' has 'operator=' but lack of 'copy constructor'.\n", errout.str()); + checkRuleOf3("class A {\n" + " A& operator=(const A& other) { return *this; }\n" + " int x;\n" + "};"); + ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' defines 'operator=' but does not define 'copy constructor' and 'destructor'\n", errout.str()); - checkCopyCtorAndEqOperator("class A \n" - "{ \n" - " A& operator=(const int &x) { this->x = x; return *this; }\n" - " int x;\n" - "};"); + checkRuleOf3("class A {\n" + " A& operator=(const int &x) { this->x = x; return *this; }\n" + " int x;\n" + "};"); ASSERT_EQUALS("", errout.str()); - checkCopyCtorAndEqOperator("class A {\n" - "public:\n" - " A() : x(0) { }\n" - " A(const A & a) { x = a.x; }\n" - " A & operator = (const A & a) {\n" - " x = a.x;\n" - " return *this;\n" - " }\n" - "private:\n" - " int x;\n" - "};\n" - "class B : public A {\n" - "public:\n" - " B() { }\n" - " B(const B & b) :A(b) { }\n" - "private:\n" - " static int i;\n" - "};"); + checkRuleOf3("class A {\n" + "public:\n" + " A() : x(0) { }\n" + " ~A() {}\n" + " A(const A & a) { x = a.x; }\n" + " A & operator = (const A & a) {\n" + " x = a.x;\n" + " return *this;\n" + " }\n" + "private:\n" + " int x;\n" + "};\n" + "class B : public A {\n" + "public:\n" + " B() { }\n" + " B(const B & b) :A(b) { }\n" + "private:\n" + " static int i;\n" + "};"); ASSERT_EQUALS("", errout.str()); // #7987 - Don't show warning when there is a move constructor - checkCopyCtorAndEqOperator("struct S {\n" - " std::string test;\n" - " S(S&& s) : test(std::move(s.test)) { }\n" - " S& operator = (S &&s) {\n" - " test = std::move(s.test);\n" - " return *this;\n" - " }\n" - "};\n"); + checkRuleOf3("struct S {\n" + " std::string test;\n" + " S(S&& s) : test(std::move(s.test)) { }\n" + " S& operator = (S &&s) {\n" + " test = std::move(s.test);\n" + " return *this;\n" + " }\n" + "};\n"); ASSERT_EQUALS("", errout.str()); // #8337 - False positive in copy constructor detection - checkCopyCtorAndEqOperator("struct StaticListNode {\n" - " StaticListNode(StaticListNode*& prev) : m_next(0) {}\n" - " StaticListNode* m_next;\n" - "};"); + checkRuleOf3("struct StaticListNode {\n" + " StaticListNode(StaticListNode*& prev) : m_next(0) {}\n" + " StaticListNode* m_next;\n" + "};"); ASSERT_EQUALS("", errout.str()); }