diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 9de173e1c0b..5bb3518dc1c 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2291,3 +2291,45 @@ void CheckClass::duplInheritedMembersError(const Token *tok1, const Token* tok2, std::string(baseIsStruct ? "struct" : "class") + " '" + basename + "'."; reportError(toks, Severity::warning, "duplInheritedMember", message, 0U, false); } + + +//--------------------------------------------------------------------------- +// Check for members hiding inherited members with the same name +//--------------------------------------------------------------------------- + +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]; + + 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) { + 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 dff80bee303..fb7b55ec105 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -77,6 +77,7 @@ class CPPCHECKLIB CheckClass : public Check { checkClass.checkDuplInheritedMembers(); checkClass.checkExplicitConstructors(); + checkClass.checkCopyCtorAndEqOperator(); } @@ -136,6 +137,9 @@ class CPPCHECKLIB CheckClass : public Check { /** @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 @@ class CPPCHECKLIB CheckClass : public Check { 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(0, settings, errorLogger); @@ -196,6 +201,7 @@ class CPPCHECKLIB CheckClass : public Check { c.suggestInitializationList(0, "variable"); c.selfInitializationError(0, "var"); c.duplInheritedMembersError(0, 0, "class", "class", "variable", false, false); + c.copyCtorAndEqOperatorError(0, "class", false, false); } static std::string myName() { @@ -221,7 +227,8 @@ class CPPCHECKLIB CheckClass : public Check { "- 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 86cc2119398..d29bedcc92a 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -184,8 +184,52 @@ class TestClass : public TestFixture { 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("[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" + "};"); + ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' has 'operator=' but lack of 'copy constructor'.\n", errout.str()); + } void checkExplicitConstructors(const char code[]) { // Clear the error log