Fixed #1184 (improve test: operator =)

This commit is contained in:
Robert Reif 2010-01-03 08:26:02 +01:00 committed by Daniel Marjamäki
parent 1032eb2449
commit 2a546dc9c0
2 changed files with 458 additions and 94 deletions

View File

@ -836,7 +836,8 @@ void CheckClass::operatorEqRetRefThis()
// check for return of reference to this // check for return of reference to this
if (tok1->str() == "return") 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); operatorEqRetRefThisError(tok);
} }
} }
@ -887,7 +888,8 @@ void CheckClass::operatorEqRetRefThis()
// check for return of reference to this // check for return of reference to this
if (tok1->str() == "return") 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); 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 // 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 // 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() void CheckClass::operatorEqToSelf()
{ {
const Token *tok2 = _tokenizer->tokens(); const Token *tok2 = _tokenizer->tokens();
@ -922,9 +1060,9 @@ void CheckClass::operatorEqToSelf()
{ {
const Token *tok1 = tok; const Token *tok1 = tok;
// make sure this is an assignment operator
if (tok1->tokAt(-2) && Token::Match(tok1->tokAt(-2), " %type% ::")) if (tok1->tokAt(-2) && Token::Match(tok1->tokAt(-2), " %type% ::"))
{ {
// make sure this is an assignment operator
int nameLength = 1; int nameLength = 1;
tok1 = tok1->tokAt(-2); tok1 = tok1->tokAt(-2);
@ -941,38 +1079,34 @@ void CheckClass::operatorEqToSelf()
nameStr(name, nameLength, nameString); nameStr(name, nameLength, nameString);
if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&") if (!hasMultipleInheritanceGlobal(_tokenizer->tokens(), nameString))
{ {
// check class name if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&")
if (tok1->tokAt(-(1 + nameLength)) && nameMatch(name, tok1->tokAt(-(1 + nameLength)), nameLength))
{ {
// check forward for proper function signature // check returned class name
std::string pattern = "const " + nameString + " & %type% )"; if (tok1->tokAt(-(1 + nameLength)) && nameMatch(name, tok1->tokAt(-(1 + nameLength)), nameLength))
if (Token::Match(tok->tokAt(3), pattern.c_str()))
{ {
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; tok1 = tok->tokAt(2)->link();
const Token *tok3 = tok->tokAt(2)->link()->next()->next();
const Token *last = tok->tokAt(2)->link()->next()->link(); if (tok1 && tok1->tokAt(1) && tok1->tokAt(1)->str() == "{" && tok1->tokAt(1)->link())
while (tok3 != last)
{ {
if (Token::Match(tok3, "if ( this ==|!= & %var% )") || const Token *first = tok1->tokAt(1);
Token::Match(tok3, "if ( & %var% ==|!= this )")) const Token *last = first->link();
if (!hasAssignSelf(first, last, rhs))
{ {
checkAssignSelf = true; if (hasDeallocation(first, last))
break; operatorEqToSelfError(tok);
} }
tok3 = tok3->next();
} }
if (!checkAssignSelf)
operatorEqToSelfError(tok);
} }
} }
} }
@ -981,62 +1115,44 @@ void CheckClass::operatorEqToSelf()
} }
else else
{ {
// make sure this is an assignment operator
tok1 = tok; tok1 = tok;
// check backwards for proper function signature // check backwards for proper function signature
if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&") if (tok1->tokAt(-1) && tok1->tokAt(-1)->str() == "&")
{ {
const Token *name = 0; const Token *name = 0;
bool isPublic = false;
while (tok1 && !Token::Match(tok1, "class|struct %var%")) while (tok1 && !Token::Match(tok1, "class|struct %var%"))
{
if (!isPublic)
{
if (tok1->str() == "public:")
isPublic = true;
}
tok1 = tok1->previous(); tok1 = tok1->previous();
}
if (tok1 && Token::Match(tok1, "struct %var%")) if (tok1 && Token::Match(tok1, "struct %var%"))
{
isPublic = true;
name = tok1->tokAt(1); name = tok1->tokAt(1);
}
else if (tok1 && Token::Match(tok1, "class %var%")) else if (tok1 && Token::Match(tok1, "class %var%"))
{
name = tok1->tokAt(1); name = tok1->tokAt(1);
}
if (tok->tokAt(-2) && tok->tokAt(-2)->str() == name->str()) if (!hasMultipleInheritanceInline(tok1))
{ {
// check forward for proper function signature if (tok->tokAt(-2) && tok->tokAt(-2)->str() == name->str())
if (Token::Match(tok->tokAt(3), "const %type% & %type% )"))
{ {
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; tok1 = tok->tokAt(2)->link();
const Token *tok3 = tok->tokAt(2)->link()->next()->next();
const Token *last = tok->tokAt(2)->link()->next()->link(); if (tok1 && tok1->tokAt(1) && tok1->tokAt(1)->str() == "{" && tok1->tokAt(1)->link())
while (tok3 != last)
{ {
if (Token::Match(tok3, "if ( this ==|!= & %var% )") || const Token *first = tok1->tokAt(1);
Token::Match(tok3, "if ( & %var% ==|!= this )")) const Token *last = first->link();
if (!hasAssignSelf(first, last, rhs))
{ {
checkAssignSelf = true; if (hasDeallocation(first, last))
break; operatorEqToSelfError(tok);
} }
tok3 = tok3->next();
} }
if (!checkAssignSelf)
operatorEqToSelfError(tok);
} }
} }
} }

View File

@ -67,7 +67,10 @@ private:
TEST_CASE(operatorEq1); TEST_CASE(operatorEq1);
TEST_CASE(operatorEqRetRefThis); 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(memsetOnStruct);
TEST_CASE(memsetOnClass); TEST_CASE(memsetOnClass);
@ -289,25 +292,45 @@ private:
checkClass.operatorEqToSelf(); checkClass.operatorEqToSelf();
} }
void operatorEqToSelf() void operatorEqToSelf1()
{ {
// this test has an assignment test but it is not needed
checkOpertorEqToSelf( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
"public:\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"); "};\n");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
// this test doesn't have an assignment test but it is not needed
checkOpertorEqToSelf( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
"public:\n" "public:\n"
" A & operator=(const A &a) { return *this; }\n" " A & operator=(const A &a) { return *this; }\n"
"};\n"); "};\n");
ASSERT_EQUALS("[test.cpp:4]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); ASSERT_EQUALS("", errout.str());
TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class
// 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( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
@ -322,24 +345,17 @@ private:
"};\n"); "};\n");
ASSERT_EQUALS("[test.cpp:5]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); 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( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
"public:\n" "public:\n"
" A & operator=(const A &);\n" " A & operator=(const A &);\n"
"};\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());
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()); ASSERT_EQUALS("", errout.str());
// this test doesn't have an assignment test but doesn't need it
checkOpertorEqToSelf( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
@ -347,70 +363,303 @@ private:
" A & operator=(const A &)\n" " A & operator=(const A &)\n"
"};\n" "};\n"
"A & A::operator=(const A &a) { return *this; }\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()); ASSERT_EQUALS("", errout.str());
TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class
// this test needs an assignment test and has it
checkOpertorEqToSelf( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
"public:\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" " 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" "};\n"
"A & A::operator=(const A &a) { return *this; }\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()); ASSERT_EQUALS("", errout.str());
TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class
// 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( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
"public:\n" "public:\n"
" class B\n" " class B : public C, public D\n"
" {\n" " {\n"
" public:\n" " public:\n"
" B & operator=(const B &b) { if (&b == this) return *this; }\n" " B & operator=(const B &b) { return *this; }\n"
" };\n" " };\n"
"};\n"); "};\n");
ASSERT_EQUALS("", errout.str()); 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( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
"public:\n" "public:\n"
" class B\n" " class B : public C, public D\n"
" {\n" " {\n"
" public:\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"
"};\n"); "};\n");
ASSERT_EQUALS("[test.cpp:7]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); ASSERT_EQUALS("", errout.str());
TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class
// this test has multiple inheritance so there is no trivial way to test for self assignment but doesn't need it
checkOpertorEqToSelf( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
"public:\n" "public:\n"
" class B\n" " class B : public C, public D\n"
" {\n" " {\n"
" public:\n" " public:\n"
" B & operator=(const B &);\n" " B & operator=(const B &);\n"
" };\n" " };\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()); 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( checkOpertorEqToSelf(
"class A\n" "class A\n"
"{\n" "{\n"
"public:\n" "public:\n"
" class B\n" " class B : public C, public D\n"
" {\n" " {\n"
" public:\n" " public:\n"
" char * s;\n"
" B & operator=(const B &);\n" " B & operator=(const B &);\n"
" };\n" " };\n"
"};\n" "};\n"
"A::B & A::B::operator=(const A::B &b) { return b; }\n"); "A::B & A::B::operator=(const A::B &b)\n"
ASSERT_EQUALS("[test.cpp:10]: (possible style) 'operator=' should check for assignment to self\n", errout.str()); "{\n"
TODO_ASSERT_EQUALS("", errout.str()); // No reason to check for self-assignment in this class " free(s);\n"
" s = strdup(b.s);\n"
" return *this;\n"
"}\n");
ASSERT_EQUALS("", errout.str());
} }
// Check that base classes have virtual destructors // Check that base classes have virtual destructors
@ -431,7 +680,6 @@ private:
checkClass.virtualDestructor(); checkClass.virtualDestructor();
} }
void virtualDestructor1() void virtualDestructor1()
{ {
// Base class not found // Base class not found