diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index b864d162d..cd650a981 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1461,7 +1461,7 @@ void CheckClass::noMemset() //--------------------------------------------------------------------------- -// ClassCheck: "void operator=(" +// ClassCheck: "void operator=(" and "const type & operator=(" //--------------------------------------------------------------------------- void CheckClass::operatorEq() @@ -1483,6 +1483,8 @@ void CheckClass::operatorEq() { if (it->token->strAt(-2) == "void") operatorEqReturnError(it->token->tokAt(-2)); + else if (Token::Match(it->token->tokAt(-4), "const %type% &")) + operatorEqReturnConstError(it->token->tokAt(-4)); } } } @@ -1493,6 +1495,56 @@ void CheckClass::operatorEq() // operator= should return a reference to *this //--------------------------------------------------------------------------- +void CheckClass::checkReturnPtrThis(const SpaceInfo *info, const Func *func, const Token *tok, const Token *last) +{ + bool foundReturn = false; + + for (; tok && tok != last; tok = tok->next()) + { + // check for return of reference to this + if (tok->str() == "return") + { + foundReturn = true; + std::string cast("( " + info->className + " & )"); + if (Token::Match(tok->next(), cast.c_str())) + tok = tok->tokAt(4); + + // check if a function is called + if (Token::Match(tok->tokAt(1), "%any% (") && + tok->tokAt(2)->link()->next()->str() == ";") + { + std::list::const_iterator it; + + // check if it is a member function + for (it = info->functionList.begin(); it != info->functionList.end(); ++it) + { + // check for a regular function with the same name and a bofy + if (it->type == Func::Function && it->hasBody && + it->token->str() == tok->next()->str()) + { + // check for the proper return type + if (it->tokenDef->previous()->str() == "&" && + it->tokenDef->strAt(-2) == info->className) + { + // make sure it's not a const function + if (it->arg->link()->next()->str() != "const") + checkReturnPtrThis(info, &*it, it->arg->link()->next(), it->arg->link()->next()->link()); + } + } + } + } + + // check of *this is returned + else if (!(Token::Match(tok->tokAt(1), "(| * this ;|=") || + Token::Match(tok->tokAt(1), "(| * this +=") || + Token::Match(tok->tokAt(1), "operator = ("))) + operatorEqRetRefThisError(func->token); + } + } + if (!foundReturn) + operatorEqRetRefThisError(func->token); +} + void CheckClass::operatorEqRetRefThis() { if (!_settings->_checkCodingStyle) @@ -1518,26 +1570,7 @@ void CheckClass::operatorEqRetRefThis() // find the ')' const Token *tok = it->token->next()->link(); - bool foundReturn = false; - const Token *last = tok->next()->link(); - for (tok = tok->tokAt(2); tok && tok != last; tok = tok->next()) - { - // check for return of reference to this - if (tok->str() == "return") - { - foundReturn = true; - std::string cast("( " + info->className + " & )"); - if (Token::Match(tok->next(), cast.c_str())) - tok = tok->tokAt(4); - - if (!(Token::Match(tok->tokAt(1), "(| * this ;|=") || - Token::Match(tok->tokAt(1), "(| * this +=") || - Token::Match(tok->tokAt(1), "operator = ("))) - operatorEqRetRefThisError(it->token); - } - } - if (!foundReturn) - operatorEqRetRefThisError(it->token); + checkReturnPtrThis(info, &*it, tok->tokAt(2), tok->next()->link()); } } } @@ -2196,6 +2229,11 @@ void CheckClass::operatorEqReturnError(const Token *tok) reportError(tok, Severity::style, "operatorEq", "'operator=' should return something"); } +void CheckClass::operatorEqReturnConstError(const Token *tok) +{ + reportError(tok, Severity::style, "operatorEqReturnConst", "'operator=' should not return a const reference"); +} + void CheckClass::virtualDestructorError(const Token *tok, const std::string &Base, const std::string &Derived) { reportError(tok, Severity::error, "virtualDestructor", "Class " + Base + " which is inherited by class " + Derived + " does not have a virtual destructor"); diff --git a/lib/checkclass.h b/lib/checkclass.h index 135e2de51..dec5da488 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -87,7 +87,7 @@ public: */ void noMemset(); - /** @brief 'operator=' should return something. */ + /** @brief 'operator=' should return something and it should not be const. */ void operatorEq(); /** @brief 'operator=' should return reference to *this */ @@ -318,6 +318,7 @@ private: void memsetClassError(const Token *tok, const std::string &memfunc); void memsetStructError(const Token *tok, const std::string &memfunc, const std::string &classname); void operatorEqReturnError(const Token *tok); + void operatorEqReturnConstError(const Token *tok); void virtualDestructorError(const Token *tok, const std::string &Base, const std::string &Derived); void thisSubtractionError(const Token *tok); void operatorEqRetRefThisError(const Token *tok); @@ -335,6 +336,7 @@ private: memsetClassError(0, "memfunc"); memsetStructError(0, "memfunc", "classname"); operatorEqReturnError(0); + operatorEqReturnConstError(0); virtualDestructorError(0, "Base", "Derived"); thisSubtractionError(0); operatorEqRetRefThisError(0); @@ -359,6 +361,9 @@ private: "* 'operator=' should check for assignment to self\n" "* Constness for member functions\n"; } + +private: + void checkReturnPtrThis(const SpaceInfo *info, const Func *func, const Token *tok, const Token *last); }; /// @} //--------------------------------------------------------------------------- diff --git a/test/testclass.cpp b/test/testclass.cpp index f6a4d1505..c5c653ec2 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -88,6 +88,7 @@ private: TEST_CASE(noConstructor5); TEST_CASE(operatorEq1); + TEST_CASE(operatorEq2); TEST_CASE(operatorEqRetRefThis1); TEST_CASE(operatorEqRetRefThis2); // ticket #1323 TEST_CASE(operatorEqRetRefThis3); // ticket #1405 @@ -99,6 +100,7 @@ private: TEST_CASE(operatorEqToSelf4); // nested class with multiple inheritance TEST_CASE(operatorEqToSelf5); // ticket # 1233 TEST_CASE(operatorEqToSelf6); // ticket # 1550 + TEST_CASE(operatorEqToSelf7); TEST_CASE(memsetOnStruct); TEST_CASE(memsetVector); TEST_CASE(memsetOnClass); @@ -230,6 +232,16 @@ private: ASSERT_EQUALS("[test.cpp:3]: (style) 'operator=' should return something\n", errout.str()); } + void operatorEq2() + { + checkOpertorEq("class A\n" + "{\n" + "public:\n" + " const A& operator=(const A&);\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) 'operator=' should not return a const reference\n", errout.str()); + } + // Check that operator Equal returns reference to this void checkOpertorEqRetRefThis(const char code[]) { @@ -1205,6 +1217,24 @@ private: ASSERT_EQUALS("[test.cpp:8]: (style) 'operator=' should check for assignment to self\n", errout.str()); } + void operatorEqToSelf7() + { + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " A & assign(const A & a)\n" + " {\n" + " return *this;\n" + " }\n" + " A & operator=(const A &a)\n" + " {\n" + " return assign(a);\n" + " }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + } + // Check that base classes have virtual destructors void checkVirtualDestructor(const char code[]) {