diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index b2d180376..fb1522e33 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -759,6 +759,298 @@ void CheckClass::operatorEq() +//--------------------------------------------------------------------------- +// ClassCheck: "C& operator=(const C&) { ... return *this; }" +// operator= should return a reference to *this +//--------------------------------------------------------------------------- + +// match two lists of tokens +static bool nameMatch(const Token * tok1, const Token * tok2, int length) +{ + for (int i = 0; i < length; i++) + { + if (tok1->tokAt(i) == 0 || tok2->tokAt(i) == 0) + return false; + + if (tok1->tokAt(i)->str() != tok2->tokAt(i)->str()) + return false; + } + + return true; +} + +// create a class name from a list of tokens +static void nameStr(const Token * name, int length, std::string & str) +{ + for (int i = 0; i < length; i++) + { + if (i != 0) + str += " "; + + str += name->tokAt(i)->str(); + } +} + +void CheckClass::operatorEqRetRefThis() +{ + const Token *tok2 = _tokenizer->tokens(); + const Token *tok; + + while ((tok = Token::findmatch(tok2, "operator = ("))) + { + const Token *tok1 = tok; + + if (tok1->tokAt(-2) && Token::Match(tok1->tokAt(-2), " %type% ::")) + { + // make sure this is an assignment operator + int nameLength = 1; + + tok1 = tok1->tokAt(-2); + + // check backwards for proper function signature + while (tok1->tokAt(-2) && Token::Match(tok1->tokAt(-2), " %type% ::")) + { + tok1 = tok1->tokAt(-2); + nameLength += 2; + } + + const Token *name = tok1; + std::string nameString; + + nameStr(name, nameLength, nameString); + + if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&") + { + // check class name + if (tok1->tokAt(-(1 + nameLength)) && nameMatch(name, tok1->tokAt(-(1 + nameLength)), nameLength)) + { + // may take overloaded argument types so no need to check them + tok1 = tok->tokAt(2)->link(); + + if (tok1 && tok1->next() && tok1->next()->str() == "{") + { + const Token *last = tok1->next()->link()->tokAt(-2); + for (tok1 = tok1->tokAt(2); tok1 != last; tok1 = tok1->next()) + { + // check for return of reference to this + if (tok1->str() == "return") + { + if (!Token::Match(tok1->tokAt(1), "* this ;")) + operatorEqRetRefThisError(tok); + } + } + } + } + } + } + else + { + // make sure this is an assignment operator + tok1 = tok; + + // check backwards for proper function signature + if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&") + { + const Token *name = 0; + bool isPublic = false; + while (tok1 && !Token::Match(tok1, "class|struct %var%")) + { + if (!isPublic) + { + if (tok1->str() == "public:") + isPublic = true; + } + tok1 = tok1->previous(); + } + + if (tok1 && Token::Match(tok1, "struct %var%")) + { + isPublic = true; + name = tok1->tokAt(1); + } + else if (tok1 && Token::Match(tok1, "class %var%")) + { + name = tok1->tokAt(1); + } + + if (tok->tokAt(-2) && tok->tokAt(-2)->str() == name->str()) + { + // may take overloaded argument types so no need to check them + tok1 = tok->tokAt(2)->link(); + + if (tok1 && tok1->next() && tok1->next()->str() == "{") + { + const Token *last = tok1->next()->link()->tokAt(-2); + for (tok1 = tok1->tokAt(2); tok1 != last; tok1 = tok1->next()) + { + // check for return of reference to this + if (tok1->str() == "return") + { + if (!Token::Match(tok1->tokAt(1), "* this ;")) + operatorEqRetRefThisError(tok); + } + } + } + } + } + } + + tok2 = tok->next(); + } +} +//--------------------------------------------------------------------------- + + + + +//--------------------------------------------------------------------------- +// ClassCheck: "C& operator=(const C& rhs) { if (this == &rhs) }" +// operator= should check for assignment to self +// For simple classes, an assignment to self check is only an optimization. +// For classes that allocate dynamic memory, assignment to self is a real error. +// This check is not valid for classes with multiple inheritance because a +// class can have multiple addresses. This should be checked for someday. +//--------------------------------------------------------------------------- + +void CheckClass::operatorEqToSelf() +{ + const Token *tok2 = _tokenizer->tokens(); + const Token *tok; + + while ((tok = Token::findmatch(tok2, "operator = ("))) + { + const Token *tok1 = tok; + + if (tok1->tokAt(-2) && Token::Match(tok1->tokAt(-2), " %type% ::")) + { + // make sure this is an assignment operator + int nameLength = 1; + + tok1 = tok1->tokAt(-2); + + // check backwards for proper function signature + while (tok1->tokAt(-2) && Token::Match(tok1->tokAt(-2), " %type% ::")) + { + tok1 = tok1->tokAt(-2); + nameLength += 2; + } + + const Token *name = tok1; + std::string nameString; + + nameStr(name, nameLength, nameString); + + if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&") + { + // check class name + if (tok1->tokAt(-(1 + nameLength)) && nameMatch(name, tok1->tokAt(-(1 + nameLength)), nameLength)) + { + // check forward for proper function signature + std::string pattern = "const " + nameString + " & %type% )"; + if (Token::Match(tok->tokAt(3), pattern.c_str())) + { + if (nameMatch(name, tok->tokAt(4), nameLength)) + { + tok1 = tok->tokAt(2)->link(); + + if (tok1 && tok1->next() && tok1->next()->str() == "{") + { + bool checkAssignSelf = false; + const Token *tok3 = tok->tokAt(2)->link()->next()->next(); + const Token *last = tok->tokAt(2)->link()->next()->link(); + while (tok3 != last) + { + if (Token::Match(tok3, "if ( this ==|!= & %var% )") || + Token::Match(tok3, "if ( & %var% ==|!= this )")) + { + checkAssignSelf = true; + break; + } + + tok3 = tok3->next(); + } + + if (!checkAssignSelf) + operatorEqToSelfError(tok); + } + } + } + } + } + } + else + { + // make sure this is an assignment operator + tok1 = tok; + + // check backwards for proper function signature + if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&") + { + const Token *name = 0; + bool isPublic = false; + while (tok1 && !Token::Match(tok1, "class|struct %var%")) + { + if (!isPublic) + { + if (tok1->str() == "public:") + isPublic = true; + } + tok1 = tok1->previous(); + } + + if (tok1 && Token::Match(tok1, "struct %var%")) + { + isPublic = true; + name = tok1->tokAt(1); + } + else if (tok1 && Token::Match(tok1, "class %var%")) + { + name = tok1->tokAt(1); + } + + if (tok->tokAt(-2) && tok->tokAt(-2)->str() == name->str()) + { + // check forward for proper function signature + if (Token::Match(tok->tokAt(3), "const %type% & %type% )")) + { + if (tok->tokAt(4)->str() == name->str()) + { + tok1 = tok->tokAt(2)->link(); + + if (tok1 && tok1->next() && tok1->next()->str() == "{") + { + bool checkAssignSelf = false; + const Token *tok3 = tok->tokAt(2)->link()->next()->next(); + const Token *last = tok->tokAt(2)->link()->next()->link(); + while (tok3 != last) + { + if (Token::Match(tok3, "if ( this ==|!= & %var% )") || + Token::Match(tok3, "if ( & %var% ==|!= this )")) + { + checkAssignSelf = true; + break; + } + + tok3 = tok3->next(); + } + + if (!checkAssignSelf) + operatorEqToSelfError(tok); + } + } + } + } + } + } + + tok2 = tok->next(); + } +} +//--------------------------------------------------------------------------- + + + + //--------------------------------------------------------------------------- // A destructor in a base class should be virtual //--------------------------------------------------------------------------- @@ -962,4 +1254,13 @@ void CheckClass::virtualDestructorError(const Token *tok, const std::string &Bas reportError(tok, Severity::error, "virtualDestructor", "Class " + Base + " which is inherited by class " + Derived + " does not have a virtual destructor"); } +void CheckClass::operatorEqRetRefThisError(const Token *tok) +{ + reportError(tok, Severity::style, "operatorEqRetRefThis", "'operator=' should return reference to self"); +} + +void CheckClass::operatorEqToSelfError(const Token *tok) +{ + reportError(tok, Severity::possibleStyle, "operatorEqToSelf", "'operator=' should check for assignment to self"); +} diff --git a/lib/checkclass.h b/lib/checkclass.h index e8bbec26b..6037cc160 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -57,8 +57,12 @@ public: checkClass.constructors(); checkClass.operatorEq(); checkClass.privateFunctions(); + checkClass.operatorEqRetRefThis(); if (settings->_showAll) + { checkClass.thisSubtraction(); + checkClass.operatorEqToSelf(); + } } checkClass.virtualDestructor(); } @@ -78,6 +82,12 @@ public: // 'operator=' should return something.. void operatorEq(); // Warning upon "void operator=(.." + // 'operator=' should return reference to *this + void operatorEqRetRefThis(); // Warning upon no "return *this;" + + // 'operator=' should check for assignment to self + void operatorEqToSelf(); // Warning upon no check for assignment to self + // The destructor in a base class should be virtual void virtualDestructor(); @@ -117,6 +127,8 @@ private: void operatorEqReturnError(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); + void operatorEqToSelfError(const Token *tok); void getErrorMessages() { @@ -129,6 +141,8 @@ private: operatorEqReturnError(0); virtualDestructorError(0, "Base", "Derived"); thisSubtractionError(0); + operatorEqRetRefThisError(0); + operatorEqToSelfError(0); } std::string name() const diff --git a/test/testclass.cpp b/test/testclass.cpp index 10f0f3d7d..0a7cf7862 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -66,6 +66,8 @@ private: TEST_CASE(noConstructor4); TEST_CASE(operatorEq1); + TEST_CASE(operatorEqRetRefThis); + TEST_CASE(operatorEqToSelf); TEST_CASE(memsetOnStruct); TEST_CASE(memsetOnClass); @@ -96,20 +98,20 @@ private: "{\n" "public:\n" " void goo() {}" - " void operator=(const& A);\n" + " void operator=(const A&);\n" "};\n"); ASSERT_EQUALS("[test.cpp:4]: (style) 'operator=' should return something\n", errout.str()); checkOpertorEq("class A\n" "{\n" "private:\n" - " void operator=(const& A);\n" + " void operator=(const A&);\n" "};\n"); ASSERT_EQUALS("", errout.str()); checkOpertorEq("class A\n" "{\n" - " void operator=(const& A);\n" + " void operator=(const A&);\n" "};\n"); ASSERT_EQUALS("", errout.str()); @@ -118,31 +120,280 @@ private: "public:\n" " void goo() {}\n" "private:\n" - " void operator=(const& A);\n" + " void operator=(const A&);\n" "};\n"); ASSERT_EQUALS("", errout.str()); checkOpertorEq("class A\n" "{\n" "public:\n" - " void operator=(const& A);\n" + " void operator=(const A&);\n" "};\n" "class B\n" "{\n" "public:\n" - " void operator=(const& B);\n" + " void operator=(const B&);\n" "};\n"); ASSERT_EQUALS("[test.cpp:4]: (style) 'operator=' should return something\n" "[test.cpp:9]: (style) 'operator=' should return something\n", errout.str()); checkOpertorEq("struct A\n" "{\n" - " void operator=(const& A);\n" + " void operator=(const A&);\n" "};\n"); ASSERT_EQUALS("[test.cpp:3]: (style) 'operator=' should return something\n", errout.str()); } + // Check that operator Equal returns reference to this + void checkOpertorEqRetRefThis(const char code[]) + { + // Tokenize.. + Tokenizer tokenizer; + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + tokenizer.simplifyTokenList(); + + // Clear the error log + errout.str(""); + + // Check.. + Settings settings; + settings._checkCodingStyle = true; + CheckClass checkClass(&tokenizer, &settings, this); + checkClass.operatorEqRetRefThis(); + } + + void operatorEqRetRefThis() + { + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &a) { return *this; }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &a) { return a; }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) 'operator=' should return reference to self\n", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a) { return *this; }\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &a);\n" + "};\n" + "A & A::operator=(const A &a) { return *this; }\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &)\n" + "};\n" + "A & A::operator=(const A &a) { return a; }\n"); + ASSERT_EQUALS("[test.cpp:6]: (style) 'operator=' should return reference to self\n", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &a)\n" + "};\n" + "A & A::operator=(const A &a) { return a; }\n"); + ASSERT_EQUALS("[test.cpp:6]: (style) 'operator=' should return reference to self\n", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " B & operator=(const B &b) { return *this; }\n" + " };\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " B & operator=(const B &b) { return b; }\n" + " };\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:7]: (style) 'operator=' should return reference to self\n", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " B & operator=(const B &);\n" + " };\n" + "};\n" + "A::B & A::B::operator=(const A::B &b) { return *this; }\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqRetRefThis( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " B & operator=(const B &);\n" + " };\n" + "};\n" + "A::B & A::B::operator=(const A::B &b) { return b; }\n"); + ASSERT_EQUALS("[test.cpp:10]: (style) 'operator=' should return reference to self\n", errout.str()); + } + + // Check that operator Equal checks for assignment to self + void checkOpertorEqToSelf(const char code[]) + { + // Tokenize.. + Tokenizer tokenizer; + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + tokenizer.simplifyTokenList(); + + // Clear the error log + errout.str(""); + + // Check.. + Settings settings; + settings._checkCodingStyle = true; + settings._showAll = true; + CheckClass checkClass(&tokenizer, &settings, this); + checkClass.operatorEqToSelf(); + } + + void operatorEqToSelf() + { + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &a) { if (&a == this) return *this; }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &a) { return *this; }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:4]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a) { if (&a == this) return *this; }\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &a);\n" + "};\n" + "A & A::operator=(const A &a) { if (&a == this) return *this; }\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &)\n" + "};\n" + "A & A::operator=(const A &a) { return *this; }\n"); + ASSERT_EQUALS("[test.cpp:6]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " A & operator=(const A &a)\n" + "};\n" + "A & A::operator=(const A &a) { return *this; }\n"); + ASSERT_EQUALS("[test.cpp:6]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " B & operator=(const B &b) { if (&b == this) return *this; }\n" + " };\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " B & operator=(const B &b) { return b; }\n" + " };\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:7]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " B & operator=(const B &);\n" + " };\n" + "};\n" + "A::B & A::B::operator=(const A::B &b) { if (&b == this) return *this; }\n"); + ASSERT_EQUALS("", errout.str()); + + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " B & operator=(const B &);\n" + " };\n" + "};\n" + "A::B & A::B::operator=(const A::B &b) { return b; }\n"); + ASSERT_EQUALS("[test.cpp:10]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + } + // Check that base classes have virtual destructors void checkVirtualDestructor(const char code[]) {