From 2a546dc9c054d1d85d35a9dda28bc26edb807243 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Sun, 3 Jan 2010 08:26:02 +0100 Subject: [PATCH] Fixed #1184 (improve test: operator =) --- lib/checkclass.cpp | 240 +++++++++++++++++++++++++--------- test/testclass.cpp | 312 ++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 458 insertions(+), 94 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 5b1684723..742abfc26 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -836,7 +836,8 @@ void CheckClass::operatorEqRetRefThis() // check for return of reference to this if (tok1->str() == "return") { - if (!Token::Match(tok1->tokAt(1), "* this ;")) + if (!(Token::Match(tok1->tokAt(1), "* this ;") || + Token::Match(tok1->tokAt(1), "operator = ("))) operatorEqRetRefThisError(tok); } } @@ -887,7 +888,8 @@ void CheckClass::operatorEqRetRefThis() // check for return of reference to this if (tok1->str() == "return") { - if (!Token::Match(tok1->tokAt(1), "* this ;")) + if (!(Token::Match(tok1->tokAt(1), "* this ;") || + Token::Match(tok1->tokAt(1), "operator = ("))) operatorEqRetRefThisError(tok); } } @@ -905,14 +907,150 @@ void CheckClass::operatorEqRetRefThis() //--------------------------------------------------------------------------- -// ClassCheck: "C& operator=(const C& rhs) { if (this == &rhs) }" +// 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. +// +// For simple classes, an assignment to self check is only a potential optimization. +// +// For classes that allocate dynamic memory, assignment to self can be a real error +// if it is deallocated and allocated again without being checked for. +// // This check is not valid for classes with multiple inheritance because a -// class can have multiple addresses. This should be checked for someday. +// class can have multiple addresses so there is no trivial way to check for +// assignment to self. //--------------------------------------------------------------------------- +static bool hasDeallocation(const Token * first, const Token * last) +{ + for (const Token * tok = first; tok && tok != last; tok = tok->next()) + { + // check for deallocating memory + if (Token::Match(tok, "{|;|, free ( %type%")) + { + const Token * var = tok->tokAt(3); + + // we should probably check that var is a pointer in this class + + tok = tok->tokAt(4); + + while (tok && tok != last) + { + if (Token::Match(tok, "%type% =")) + { + if (tok->str() == var->str()) + return true; + } + + tok = tok->next(); + } + } + else if (Token::Match(tok, "{|;|, delete [ ] %type%")) + { + const Token * var = tok->tokAt(4); + + // we should probably check that var is a pointer in this class + + tok = tok->tokAt(5); + + while (tok && tok != last) + { + if (Token::Match(tok, "%type% = new [")) + { + if (tok->str() == var->str()) + return true; + } + + tok = tok->next(); + } + } + else if (Token::Match(tok, "{|;|, delete %type%")) + { + const Token * var = tok->tokAt(2); + + // we should probably check that var is a pointer in this class + + tok = tok->tokAt(3); + + while (tok && tok != last) + { + if (Token::Match(tok, "%type% = new")) + { + if (tok->str() == var->str()) + return true; + } + + tok = tok->next(); + } + } + } + + return false; +} + +static bool hasAssignSelf(const Token * first, const Token * last, const Token * rhs) +{ + for (const Token * tok = first; tok && tok != last; tok = tok->next()) + { + if (Token::Match(tok, "if ( this ==|!= & %var% )")) + { + if (tok->tokAt(5)->str() == rhs->str()) + return true; + } + else if (Token::Match(tok, "if ( & %var% ==|!= this )")) + { + if (tok->tokAt(3)->str() == rhs->str()) + return true; + } + } + + return false; +} + +static bool hasMultipleInheritanceInline(const Token * tok) +{ + while (tok && tok->str() != "{") + { + if (tok->str() == ",") + return true; + + tok = tok->next(); + } + + return false; +} + +static bool hasMultipleInheritanceGlobal(const Token * start, const std::string & name) +{ + const Token *tok = start; + std::string pattern; + std::string className = name; + + // check for nested classes + while (className.find("::") != std::string::npos) + { + std::string tempName; + + // there is probably a better way to do this + while (className[0] != ' ') + { + tempName += className[0]; + className.erase(0, 1); + } + + className.erase(0, 4); + + pattern = "class|struct " + tempName; + + tok = Token::findmatch(tok, pattern.c_str()); + } + + pattern = "class|struct " + className; + + tok = Token::findmatch(tok, pattern.c_str()); + + return hasMultipleInheritanceInline(tok); +} + void CheckClass::operatorEqToSelf() { const Token *tok2 = _tokenizer->tokens(); @@ -922,9 +1060,9 @@ void CheckClass::operatorEqToSelf() { const Token *tok1 = tok; + // make sure this is an assignment operator if (tok1->tokAt(-2) && Token::Match(tok1->tokAt(-2), " %type% ::")) { - // make sure this is an assignment operator int nameLength = 1; tok1 = tok1->tokAt(-2); @@ -941,38 +1079,34 @@ void CheckClass::operatorEqToSelf() nameStr(name, nameLength, nameString); - if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&") + if (!hasMultipleInheritanceGlobal(_tokenizer->tokens(), nameString)) { - // check class name - if (tok1->tokAt(-(1 + nameLength)) && nameMatch(name, tok1->tokAt(-(1 + nameLength)), nameLength)) + if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&") { - // check forward for proper function signature - std::string pattern = "const " + nameString + " & %type% )"; - if (Token::Match(tok->tokAt(3), pattern.c_str())) + // check returned class name + if (tok1->tokAt(-(1 + nameLength)) && nameMatch(name, tok1->tokAt(-(1 + nameLength)), nameLength)) { - if (nameMatch(name, tok->tokAt(4), nameLength)) + // check forward for proper function signature + std::string pattern = "const " + nameString + " & %type% )"; + if (Token::Match(tok->tokAt(3), pattern.c_str())) { - tok1 = tok->tokAt(2)->link(); + const Token * rhs = tok->tokAt(5 + nameLength); - if (tok1 && tok1->next() && tok1->next()->str() == "{") + if (nameMatch(name, tok->tokAt(4), nameLength)) { - bool checkAssignSelf = false; - const Token *tok3 = tok->tokAt(2)->link()->next()->next(); - const Token *last = tok->tokAt(2)->link()->next()->link(); - while (tok3 != last) + tok1 = tok->tokAt(2)->link(); + + if (tok1 && tok1->tokAt(1) && tok1->tokAt(1)->str() == "{" && tok1->tokAt(1)->link()) { - if (Token::Match(tok3, "if ( this ==|!= & %var% )") || - Token::Match(tok3, "if ( & %var% ==|!= this )")) + const Token *first = tok1->tokAt(1); + const Token *last = first->link(); + + if (!hasAssignSelf(first, last, rhs)) { - checkAssignSelf = true; - break; + if (hasDeallocation(first, last)) + operatorEqToSelfError(tok); } - - tok3 = tok3->next(); } - - if (!checkAssignSelf) - operatorEqToSelfError(tok); } } } @@ -981,62 +1115,44 @@ void CheckClass::operatorEqToSelf() } 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()) + if (!hasMultipleInheritanceInline(tok1)) { - // check forward for proper function signature - if (Token::Match(tok->tokAt(3), "const %type% & %type% )")) + if (tok->tokAt(-2) && tok->tokAt(-2)->str() == name->str()) { - if (tok->tokAt(4)->str() == name->str()) + // check forward for proper function signature + if (Token::Match(tok->tokAt(3), "const %type% & %type% )")) { - tok1 = tok->tokAt(2)->link(); + const Token * rhs = tok->tokAt(6); - if (tok1 && tok1->next() && tok1->next()->str() == "{") + if (tok->tokAt(4)->str() == name->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) + tok1 = tok->tokAt(2)->link(); + + if (tok1 && tok1->tokAt(1) && tok1->tokAt(1)->str() == "{" && tok1->tokAt(1)->link()) { - if (Token::Match(tok3, "if ( this ==|!= & %var% )") || - Token::Match(tok3, "if ( & %var% ==|!= this )")) + const Token *first = tok1->tokAt(1); + const Token *last = first->link(); + + if (!hasAssignSelf(first, last, rhs)) { - checkAssignSelf = true; - break; + if (hasDeallocation(first, last)) + operatorEqToSelfError(tok); } - - tok3 = tok3->next(); } - - if (!checkAssignSelf) - operatorEqToSelfError(tok); } } } diff --git a/test/testclass.cpp b/test/testclass.cpp index 4022ed0ba..13fb92b15 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -67,7 +67,10 @@ private: TEST_CASE(operatorEq1); TEST_CASE(operatorEqRetRefThis); - TEST_CASE(operatorEqToSelf); + TEST_CASE(operatorEqToSelf1); // single class + TEST_CASE(operatorEqToSelf2); // nested class + TEST_CASE(operatorEqToSelf3); // multiple inheritance + TEST_CASE(operatorEqToSelf4); // nested class with multiple inheritance TEST_CASE(memsetOnStruct); TEST_CASE(memsetOnClass); @@ -289,25 +292,45 @@ private: checkClass.operatorEqToSelf(); } - void operatorEqToSelf() + void operatorEqToSelf1() { + // this test has an assignment test but it is not needed checkOpertorEqToSelf( "class A\n" "{\n" "public:\n" - " A & operator=(const A &a) { if (&a == this) return *this; }\n" + " A & operator=(const A &a) { if (&a != this) { } return *this; }\n" "};\n"); ASSERT_EQUALS("", errout.str()); + // this test doesn't have an assignment test but it is not needed 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()); - TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class + ASSERT_EQUALS("", errout.str()); + // this test needs an assignment test and has it + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &a)\n" + " {\n" + " if (&a != this)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + " }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + // this class needs an assignment test but doesn't have it checkOpertorEqToSelf( "class A\n" "{\n" @@ -322,24 +345,17 @@ private: "};\n"); ASSERT_EQUALS("[test.cpp:5]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + // this test has an assignment test but doesn't need it 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"); + "A & A::operator=(const A &a) { if (&a != this) { } return *this; }\n"); ASSERT_EQUALS("", errout.str()); + // this test doesn't have an assignment test but doesn't need it checkOpertorEqToSelf( "class A\n" "{\n" @@ -347,70 +363,303 @@ private: " 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()); - TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class + ASSERT_EQUALS("", errout.str()); + // this test needs an assignment test and has it checkOpertorEqToSelf( "class A\n" "{\n" "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & operator=(const A &a)\n" + "{\n" + " if (&a != this)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " }\n" + " return *this;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + // this test needs an assignment test but doesnt have it + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & operator=(const A &a)\n" + "{\n" + " free(s);\n" + " s = strdup(a.s);\n" + " return *this;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + } + + void operatorEqToSelf2() + { + // this test has an assignment test but doesn't need it + 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()); + + // this test doesn't have an assignment test but doesn't need it + checkOpertorEqToSelf( + "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()); + + // this test needs an assignment test but has it + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " char *s;\n" + " B & operator=(const B &b)\n" + " {\n" + " if (&b != this)\n" + " {\n" + " }\n" + " return *this;\n" + " }\n" + " };\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + // this test needs an assignment test but doesn't have it + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " char *s;\n" + " B & operator=(const B &b)\n" + " {\n" + " free(s);\n" + " s = strdup(b.s);\n" + " return *this;\n" + " }\n" + " };\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:8]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + + // this test has an assignment test but doesn't need it + 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()); + + // this test doesn't have an assignment test but doesn't need it + 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 *this; }\n"); + ASSERT_EQUALS("", errout.str()); + + // this test needs an assignment test and has it + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " char * s;\n" + " B & operator=(const B &);\n" + " };\n" + "};\n" + "A::B & A::B::operator=(const A::B &b)\n" + "{\n" + " if (&b != this)\n" + " {\n" + " free(s);\n" + " s = strdup(b.s);\n" + " }\n" + " return *this;\n" + " }\n"); + ASSERT_EQUALS("", errout.str()); + + // this test needs an assignment test but doesn't have it + checkOpertorEqToSelf( + "class A\n" + "{\n" + "public:\n" + " class B\n" + " {\n" + " public:\n" + " char * s;\n" + " B & operator=(const B &);\n" + " };\n" + "};\n" + "A::B & A::B::operator=(const A::B &b)\n" + "{\n" + " free(s);\n" + " s = strdup(b.s);\n" + " return *this;\n" + " }\n"); + ASSERT_EQUALS("[test.cpp:11]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); + } + + void operatorEqToSelf3() + { + // this test has multiple inheritance so there is no trivial way to test for self assignment but doesn't need it + checkOpertorEqToSelf( + "class A : public B, public C\n" + "{\n" + "public:\n" + " A & operator=(const A &a) { return *this; }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + // this test has multiple inheritance and needs an assignment test but there is no trivial way to test for it + checkOpertorEqToSelf( + "class A : public B, public C\n" + "{\n" + "public:\n" + " char *s;\n" " A & operator=(const A &a)\n" + " {\n" + " free(s);\n" + " s = strdup(a.s);\n" + " return *this;\n" + " }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + // this test has multiple inheritance so there is no trivial way to test for self assignment but doesn't need it + checkOpertorEqToSelf( + "class A : public B, public C\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()); - TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class + ASSERT_EQUALS("", errout.str()); + // this test has multiple inheritance and needs an assignment test but there is no trivial way to test for it + checkOpertorEqToSelf( + "class A : public B, public C\n" + "{\n" + "public:\n" + " char *s;\n" + " A & operator=(const A &);\n" + "};\n" + "A & A::operator=(const A &a)\n" + "{\n" + " free(s);\n" + " s = strdup(a.s);\n" + " return *this;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + + void operatorEqToSelf4() + { + // this test has multiple inheritance so there is no trivial way to test for self assignment but doesn't need it checkOpertorEqToSelf( "class A\n" "{\n" "public:\n" - " class B\n" + " class B : public C, public D\n" " {\n" " public:\n" - " B & operator=(const B &b) { if (&b == this) return *this; }\n" + " B & operator=(const B &b) { return *this; }\n" " };\n" "};\n"); ASSERT_EQUALS("", errout.str()); + // this test has multiple inheritance and needs an assignment test but there is no trivial way to test for it checkOpertorEqToSelf( "class A\n" "{\n" "public:\n" - " class B\n" + " class B : public C, public D\n" " {\n" " public:\n" - " B & operator=(const B &b) { return b; }\n" + " char * s;\n" + " B & operator=(const B &b)\n" + " {\n" + " free(s);\n" + " s = strdup(b.s);\n" + " return *this;\n" + " }\n" " };\n" "};\n"); - ASSERT_EQUALS("[test.cpp:7]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); - TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class + ASSERT_EQUALS("", errout.str()); + // this test has multiple inheritance so there is no trivial way to test for self assignment but doesn't need it checkOpertorEqToSelf( "class A\n" "{\n" "public:\n" - " class B\n" + " class B : public C, public D\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"); + "A::B & A::B::operator=(const A::B &b) { return *this; }\n"); ASSERT_EQUALS("", errout.str()); + // this test has multiple inheritance and needs an assignment test but there is no trivial way to test for it checkOpertorEqToSelf( "class A\n" "{\n" "public:\n" - " class B\n" + " class B : public C, public D\n" " {\n" " public:\n" + " char * s;\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()); - TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class + "A::B & A::B::operator=(const A::B &b)\n" + "{\n" + " free(s);\n" + " s = strdup(b.s);\n" + " return *this;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } // Check that base classes have virtual destructors @@ -431,7 +680,6 @@ private: checkClass.virtualDestructor(); } - void virtualDestructor1() { // Base class not found