From deaafd59d7fff58a1edf8e3d9ed84aeb84f3b997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 24 Apr 2018 22:42:25 +0200 Subject: [PATCH] CheckClass: Undo the rule of 3 checker to avoid some warnings --- lib/checkclass.cpp | 91 +++++++++++------------------ lib/checkclass.h | 12 ++-- lib/suppressions.cpp | 2 +- test/testclass.cpp | 133 ++++++++++++++++++++++--------------------- 4 files changed, 107 insertions(+), 131 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 57d877ef0..464594ca7 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2368,11 +2368,17 @@ void CheckClass::duplInheritedMembersError(const Token *tok1, const Token* tok2, } -//--------------------------------------------------------------------------------------------------------------------- -// Rule of 3: If a class defines a 'copy constructor', 'destructor' or 'operator='; then all these must be defined -//--------------------------------------------------------------------------------------------------------------------- +//--------------------------------------------------------------------------- +// Check that copy constructor and operator defined together +//--------------------------------------------------------------------------- -void CheckClass::checkRuleOf3() +enum CtorType { + NO, + WITHOUT_BODY, + WITH_BODY +}; + +void CheckClass::checkCopyCtorAndEqOperator() { if (!_settings->isEnabled(Settings::WARNING)) return; @@ -2380,8 +2386,8 @@ void CheckClass::checkRuleOf3() for (const Scope * scope : symbolDatabase->classAndStructScopes) { bool hasNonStaticVars = false; - for (const Variable &var : scope->varlist) { - if (!var.isStatic()) { + for (std::list::const_iterator var = scope->varlist.begin(); var != scope->varlist.end(); ++var) { + if (!var->isStatic()) { hasNonStaticVars = true; break; } @@ -2389,31 +2395,22 @@ void CheckClass::checkRuleOf3() if (!hasNonStaticVars) continue; - enum CtorType { - NO, - WITHOUT_BODY, - WITH_BODY - }; - - CtorType copyCtor = CtorType::NO; - CtorType assignmentOperator = CtorType::NO; - bool destructor = false; + CtorType copyCtors = CtorType::NO; bool moveCtor = false; + CtorType assignmentOperators = CtorType::NO; - for (const Function &func : scope->functionList) { - if (copyCtor != CtorType::WITH_BODY && func.type == Function::eCopyConstructor) { - copyCtor = func.hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_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; } - if (assignmentOperator != CtorType::WITH_BODY && func.type == Function::eOperatorEqual) { - const Variable * variable = func.getArgumentVar(0); + if (assignmentOperators == CtorType::NO && func->type == Function::eOperatorEqual) { + const Variable * variable = func->getArgumentVar(0); if (variable && variable->type() && variable->type()->classScope == scope) { - assignmentOperator = func.hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY; + assignmentOperators = func->hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY; } } - if (func.type == Function::eDestructor) { - destructor = true; - } - if (func.type == Function::eMoveConstructor) { + if (func->type == Function::eMoveConstructor) { moveCtor = true; break; } @@ -2423,47 +2420,25 @@ void CheckClass::checkRuleOf3() continue; // No method defined - if (copyCtor != CtorType::WITH_BODY && assignmentOperator != CtorType::WITH_BODY && !destructor) + if (copyCtors != CtorType::WITH_BODY && assignmentOperators != CtorType::WITH_BODY) continue; - // all 3 methods are defined - if (copyCtor != CtorType::NO && assignmentOperator != CtorType::NO && destructor) + // both methods are defined + if (copyCtors != CtorType::NO && assignmentOperators != CtorType::NO) continue; - ruleOf3Error(scope->classDef, scope->className, scope->type == Scope::eStruct, copyCtor != CtorType::NO, assignmentOperator != CtorType::NO, destructor); + copyCtorAndEqOperatorError(scope->classDef, scope->className, scope->type == Scope::eStruct, copyCtors == CtorType::WITH_BODY); } } -void CheckClass::ruleOf3Error(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor, bool hasAssignmentOperator, bool hasDestructor) +void CheckClass::copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor) { - 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); + 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); } void CheckClass::checkUnsafeClassDivZero(bool test) diff --git a/lib/checkclass.h b/lib/checkclass.h index d97b3ef93..af85f7a3f 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -89,7 +89,7 @@ public: checkClass.checkDuplInheritedMembers(); checkClass.checkExplicitConstructors(); - checkClass.checkRuleOf3(); + checkClass.checkCopyCtorAndEqOperator(); } @@ -149,8 +149,8 @@ public: /** @brief Check duplicated inherited members */ void checkDuplInheritedMembers(); - /** @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 copy constructor and operator defined together */ + void checkCopyCtorAndEqOperator(); /** @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 ruleOf3Error(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor, bool hasAssignmentOperator, bool hasDestructor); + void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor); 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.ruleOf3Error(nullptr, "class", false, false, true, true); + c.copyCtorAndEqOperatorError(nullptr, "class", false, false); 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" - "- Rule of 3: If a class defines a 'copy constructor', 'destructor' or 'operator='; then all these must be defined\n" + "- If 'copy constructor' defined, 'operator=' also should be defined and vice versa\n" "- Check that arbitrary usage of public interface does not result in division by zero\n"; } diff --git a/lib/suppressions.cpp b/lib/suppressions.cpp index b16ca21af..18d95f97f 100644 --- a/lib/suppressions.cpp +++ b/lib/suppressions.cpp @@ -283,7 +283,7 @@ void Suppressions::dump(std::ostream & out) for (const Suppression &suppression : _suppressions) { out << " 0) out << " lineNumber=\"" << suppression.lineNumber << '"'; diff --git a/test/testclass.cpp b/test/testclass.cpp index 1d86f8c80..21225030e 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -188,12 +188,12 @@ private: TEST_CASE(duplInheritedMembers); TEST_CASE(explicitConstructors); - TEST_CASE(ruleOf3); + TEST_CASE(copyCtorAndEqOperator); TEST_CASE(unsafeClassDivZero); } - void checkRuleOf3(const char code[]) { + void checkCopyCtorAndEqOperator(const char code[]) { // Clear the error log errout.str(""); Settings settings; @@ -207,92 +207,93 @@ private: // Check.. CheckClass checkClass(&tokenizer, &settings, this); - checkClass.checkRuleOf3(); + checkClass.checkCopyCtorAndEqOperator(); } - void ruleOf3() { - checkRuleOf3("class A {\n" - " A(const A& other) { } \n" - " A& operator=(const A& other) { return *this; }\n" - "};"); + void copyCtorAndEqOperator() { + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + " A(const A& other) { } \n" + " A& operator=(const A& other) { return *this; }\n" + "};"); ASSERT_EQUALS("", errout.str()); - checkRuleOf3("class A {};"); + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + "};"); ASSERT_EQUALS("", errout.str()); - checkRuleOf3("class A {\n" - " A(const A& other) { } \n" - "};"); + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + " A(const A& other) { } \n" + "};"); ASSERT_EQUALS("", errout.str()); - checkRuleOf3("class A {\n" - " A& operator=(const A& other) { return *this; }\n" - "};"); + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + " A& operator=(const A& other) { return *this; }\n" + "};"); ASSERT_EQUALS("", 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(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& 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 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() { }\n" - " int x;\n" - "};"); - ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' defines 'destructor' but does not define 'copy constructor' and 'operator='\n", errout.str()); - - checkRuleOf3("class A {\n" - " A& operator=(const int &x) { this->x = x; return *this; }\n" - " int x;\n" - "};"); + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + " A& operator=(const int &x) { this->x = x; return *this; }\n" + " int x;\n" + "};"); ASSERT_EQUALS("", errout.str()); - 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" - "};"); + 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" + "};"); ASSERT_EQUALS("", errout.str()); // #7987 - Don't show warning when there is a move constructor - 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"); + 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"); ASSERT_EQUALS("", errout.str()); // #8337 - False positive in copy constructor detection - checkRuleOf3("struct StaticListNode {\n" - " StaticListNode(StaticListNode*& prev) : m_next(0) {}\n" - " StaticListNode* m_next;\n" - "};"); + checkCopyCtorAndEqOperator("struct StaticListNode {\n" + " StaticListNode(StaticListNode*& prev) : m_next(0) {}\n" + " StaticListNode* m_next;\n" + "};"); ASSERT_EQUALS("", errout.str()); }