From abba762d42257a861eae68bebcef98f59710facb Mon Sep 17 00:00:00 2001 From: Alexander Alekseev Date: Fri, 24 Mar 2017 12:00:20 +0100 Subject: [PATCH] New check: checking for copy ctor and eq operator co-existence --- lib/checkclass.cpp | 48 +++++++++++++++++++++++++++++++++ lib/checkclass.h | 9 ++++++- test/testclass.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 7f662b5aa..05f8fa8bd 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2333,3 +2333,51 @@ void CheckClass::duplInheritedMembersError(const Token *tok1, const Token* tok2, std::string(baseIsStruct ? "struct" : "class") + " '" + basename + "'."; reportError(toks, Severity::warning, "duplInheritedMember", message, CWE398, false); } + + +//--------------------------------------------------------------------------- +// Check that copy constructor and operator defined together +//--------------------------------------------------------------------------- + +void CheckClass::checkCopyCtorAndEqOperator() +{ + if (!_settings->isEnabled("warning")) + return; + + const std::size_t classes = symbolDatabase->classAndStructScopes.size(); + for (std::size_t i = 0; i < classes; ++i) { + const Scope * scope = symbolDatabase->classAndStructScopes[i]; + + if (scope->varlist.empty()) + continue; + + int hasCopyCtor = 0; + int hasAssignmentOperator = 0; + + std::list::const_iterator func; + for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { + if (!hasCopyCtor && func->type == Function::eCopyConstructor) { + hasCopyCtor = func->hasBody() ? 2 : 1; + } + if (!hasAssignmentOperator && func->type == Function::eOperatorEqual) { + const Variable * variable = func->getArgumentVar(0); + if (variable && variable->type() && variable->type()->classScope == scope) { + hasAssignmentOperator = func->hasBody() ? 2 : 1; + } + } + } + + if (std::abs(hasCopyCtor - hasAssignmentOperator) == 2) + copyCtorAndEqOperatorError(scope->classDef, scope->className, scope->type == Scope::eStruct, hasCopyCtor); + } +} + +void CheckClass::copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor) +{ + const std::string message = "The " + std::string(isStruct ? "struct" : "class") + " '" + classname + + "' has '" + getFunctionTypeName(hasCopyCtor ? Function::eCopyConstructor : Function::eOperatorEqual) + + "' but lack of '" + getFunctionTypeName(hasCopyCtor ? Function::eOperatorEqual : Function::eCopyConstructor) + + "'."; + + reportError(tok, Severity::warning, "copyCtorAndEqOperator", message); +} diff --git a/lib/checkclass.h b/lib/checkclass.h index a9e55b372..a478b6f84 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -77,6 +77,7 @@ public: checkClass.checkDuplInheritedMembers(); checkClass.checkExplicitConstructors(); + checkClass.checkCopyCtorAndEqOperator(); } @@ -136,6 +137,9 @@ public: /** @brief Check duplicated inherited members */ void checkDuplInheritedMembers(); + /** @brief Check that copy constructor and operator defined together */ + void checkCopyCtorAndEqOperator(); + private: const SymbolDatabase *symbolDatabase; @@ -167,6 +171,7 @@ private: void selfInitializationError(const Token* tok, const std::string& varname); void callsPureVirtualFunctionError(const Function & scopeFunction, const std::list & tokStack, const std::string &purefuncname); 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 getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckClass c(nullptr, settings, errorLogger); @@ -196,6 +201,7 @@ private: c.suggestInitializationList(nullptr, "variable"); c.selfInitializationError(nullptr, "var"); c.duplInheritedMembersError(nullptr, 0, "class", "class", "variable", false, false); + c.copyCtorAndEqOperatorError(nullptr, "class", false, false); } static std::string myName() { @@ -221,7 +227,8 @@ private: "- Initialization of a member with itself\n" "- Suspicious subtraction from 'this'\n" "- Call of pure virtual function in constructor/destructor\n" - "- Duplicated inherited data members\n"; + "- Duplicated inherited data members\n" + "- If 'copy constructor' defined, 'operator=' also should be defined and vice versa\n"; } // operatorEqRetRefThis helper functions diff --git a/test/testclass.cpp b/test/testclass.cpp index 95b95242f..e974acef5 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -184,8 +184,74 @@ private: TEST_CASE(duplInheritedMembers); TEST_CASE(explicitConstructors); + TEST_CASE(copyCtorAndEqOperator); } + void checkCopyCtorAndEqOperator(const char code[]) { + // Clear the error log + errout.str(""); + Settings settings; + settings.addEnabled("warning"); + + // Tokenize.. + Tokenizer tokenizer(&settings, this); + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + tokenizer.simplifyTokenList2(); + + // Check.. + CheckClass checkClass(&tokenizer, &settings, this); + checkClass.checkCopyCtorAndEqOperator(); + } + + void copyCtorAndEqOperator() { + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + " A(const A& other) { } \n" + " A& operator=(const A& other) { return *this; }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + " A(const A& other) { } \n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkCopyCtorAndEqOperator("class A \n" + "{ \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()); + + 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()); + + checkCopyCtorAndEqOperator("class A \n" + "{ \n" + " A& operator=(const int &x) { this->x = x; return *this; }\n" + " int x;\n" + "};"); + ASSERT_EQUALS("", errout.str()); + } void checkExplicitConstructors(const char code[]) { // Clear the error log