walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

CheckClass: Changed checker for 'copy constructor' and 'operator=' to a 'rule of 3' checker

This commit is contained in:
Daniel Marjamäki 2018-04-24 16:07:58 +02:00
parent 8310198cd5
commit 6fb25dcaa4
3 changed files with 130 additions and 106 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

97
lib/checkclass.cpp
View File

@ -2368,17 +2368,11 @@ void CheckClass::duplInheritedMembersError(const Token *tok1, const Token* tok2,
}
//---------------------------------------------------------------------------
// Check that copy constructor and operator defined together
//---------------------------------------------------------------------------
//---------------------------------------------------------------------------------------------------------------------
// Rule of 3: If a class defines a 'copy constructor', 'destructor' or 'operator='; then all these must be defined
//---------------------------------------------------------------------------------------------------------------------
enum CtorType {
NO,
WITHOUT_BODY,
WITH_BODY
};
void CheckClass::checkCopyCtorAndEqOperator()
void CheckClass::checkRuleOf3()
{
if (!_settings->isEnabled(Settings::WARNING))
return;
@ -2386,8 +2380,8 @@ void CheckClass::checkCopyCtorAndEqOperator()
for (const Scope * scope : symbolDatabase->classAndStructScopes) {
bool hasNonStaticVars = false;
for (std::list<Variable>::const_iterator var = scope->varlist.begin(); var != scope->varlist.end(); ++var) {
if (!var->isStatic()) {
for (const Variable &var : scope->varlist) {
if (!var.isStatic()) {
hasNonStaticVars = true;
break;
}
@ -2395,22 +2389,31 @@ void CheckClass::checkCopyCtorAndEqOperator()
if (!hasNonStaticVars)
continue;
CtorType copyCtors = CtorType::NO;
bool moveCtor = false;
CtorType assignmentOperators = CtorType::NO;
enum CtorType {
NO,
WITHOUT_BODY,
WITH_BODY
};
std::list<Function>::const_iterator func;
for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
if (copyCtors == CtorType::NO && func->type == Function::eCopyConstructor) {
copyCtors = func->hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY;
CtorType copyCtor = CtorType::NO;
CtorType assignmentOperator = CtorType::NO;
bool destructor = false;
bool moveCtor = false;
for (const Function &func : scope->functionList) {
if (copyCtor != CtorType::WITH_BODY && func.type == Function::eCopyConstructor) {
copyCtor = func.hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY;
}
if (assignmentOperators == CtorType::NO && func->type == Function::eOperatorEqual) {
const Variable * variable = func->getArgumentVar(0);
if (assignmentOperator != CtorType::WITH_BODY && func.type == Function::eOperatorEqual) {
const Variable * variable = func.getArgumentVar(0);
if (variable && variable->type() && variable->type()->classScope == scope) {
assignmentOperators = func->hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY;
assignmentOperator = func.hasBody() ? CtorType::WITH_BODY : CtorType::WITHOUT_BODY;
}
}
if (func->type == Function::eMoveConstructor) {
if (func.type == Function::eDestructor) {
destructor = true;
}
if (func.type == Function::eMoveConstructor) {
moveCtor = true;
break;
}
@ -2419,20 +2422,48 @@ void CheckClass::checkCopyCtorAndEqOperator()
if (moveCtor)
continue;
if ((copyCtors == CtorType::WITH_BODY && assignmentOperators == CtorType::NO) ||
(copyCtors == CtorType::NO && assignmentOperators == CtorType::WITH_BODY))
copyCtorAndEqOperatorError(scope->classDef, scope->className, scope->type == Scope::eStruct, copyCtors == CtorType::WITH_BODY);
// No method defined
if (copyCtor != CtorType::WITH_BODY && assignmentOperator != CtorType::WITH_BODY && !destructor)
continue;
// all 3 methods are defined
if (copyCtor == CtorType::WITH_BODY && assignmentOperator == CtorType::WITH_BODY && destructor)
continue;
ruleOf3Error(scope->classDef, scope->className, scope->type == Scope::eStruct, copyCtor == CtorType::WITH_BODY, assignmentOperator == CtorType::WITH_BODY, destructor);
}
}
void CheckClass::copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor)
void CheckClass::ruleOf3Error(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor, bool hasAssignmentOperator, bool hasDestructor)
{
const std::string message = "$symbol:" + classname + "\n"
"The " + std::string(isStruct ? "struct" : "class") + " '$symbol' has '" +
getFunctionTypeName(hasCopyCtor ? Function::eCopyConstructor : Function::eOperatorEqual) +
"' but lack of '" + getFunctionTypeName(hasCopyCtor ? Function::eOperatorEqual : Function::eCopyConstructor) +
"'.";
reportError(tok, Severity::warning, "copyCtorAndEqOperator", message);
std::string message = "$symbol:" + classname + "\n"
"The " + std::string(isStruct ? "struct" : "class") + " '$symbol' defines";
if (hasCopyCtor)
message += " '" + std::string(getFunctionTypeName(Function::eCopyConstructor)) + '\'';
if (hasAssignmentOperator) {
if (hasCopyCtor)
message += " and";
message += " '" + std::string(getFunctionTypeName(Function::eOperatorEqual)) + '\'';
}
if (hasDestructor) {
if (hasCopyCtor || hasAssignmentOperator)
message += " and";
message += " '" + std::string(getFunctionTypeName(Function::eDestructor)) + '\'';
}
message += " but does not define";
if (!hasCopyCtor)
message += " '" + std::string(getFunctionTypeName(Function::eCopyConstructor)) + '\'';
if (!hasAssignmentOperator) {
if (!hasCopyCtor)
message += " and";
message += " '" + std::string(getFunctionTypeName(Function::eOperatorEqual)) + '\'';
}
if (!hasDestructor) {
if (!hasCopyCtor || !hasAssignmentOperator)
message += " and";
message += " '" + std::string(getFunctionTypeName(Function::eDestructor)) + '\'';
}
reportError(tok, Severity::warning, "ruleOf3", message);
}
void CheckClass::checkUnsafeClassDivZero(bool test)

12
lib/checkclass.h
View File

@ -89,7 +89,7 @@ public:
checkClass.checkDuplInheritedMembers();
checkClass.checkExplicitConstructors();
checkClass.checkCopyCtorAndEqOperator();
checkClass.checkRuleOf3();
}
@ -149,8 +149,8 @@ public:
/** @brief Check duplicated inherited members */
void checkDuplInheritedMembers();
/** @brief Check that copy constructor and operator defined together */
void checkCopyCtorAndEqOperator();
/** @brief Rule of 3: If a class defines a 'copy constructor', 'destructor' or 'operator='; then all these must be defined */
void checkRuleOf3();
/** @brief Check that arbitrary usage of the public interface does not result in division by zero */
void checkUnsafeClassDivZero(bool test=false);
@ -187,7 +187,7 @@ private:
void pureVirtualFunctionCallInConstructorError(const Function * scopeFunction, const std::list<const Token *> & tokStack, const std::string &purefuncname);
void virtualFunctionCallInConstructorError(const Function * scopeFunction, const std::list<const Token *> & tokStack, const std::string &funcname);
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 ruleOf3Error(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor, bool hasAssignmentOperator, bool hasDestructor);
void unsafeClassDivZeroError(const Token *tok, const std::string &className, const std::string &methodName, const std::string &varName);
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
@ -218,7 +218,7 @@ private:
c.suggestInitializationList(nullptr, "variable");
c.selfInitializationError(nullptr, "var");
c.duplInheritedMembersError(nullptr, nullptr, "class", "class", "variable", false, false);
c.copyCtorAndEqOperatorError(nullptr, "class", false, false);
c.ruleOf3Error(nullptr, "class", false, false, true, true);
c.unsafeClassDivZeroError(nullptr, "Class", "dostuff", "x");
c.pureVirtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
c.virtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
@ -248,7 +248,7 @@ private:
"- Suspicious subtraction from 'this'\n"
"- Call of pure virtual function in constructor/destructor\n"
"- Duplicated inherited data members\n"
"- If 'copy constructor' defined, 'operator=' also should be defined and vice versa\n"
"- Rule of 3: If a class defines a 'copy constructor', 'destructor' or 'operator='; then all these must be defined\n"
"- Check that arbitrary usage of public interface does not result in division by zero\n";
}

41
test/testclass.cpp
View File

@ -188,12 +188,12 @@ private:
TEST_CASE(duplInheritedMembers);
TEST_CASE(explicitConstructors);
TEST_CASE(copyCtorAndEqOperator);
TEST_CASE(ruleOf3);
TEST_CASE(unsafeClassDivZero);
}
void checkCopyCtorAndEqOperator(const char code[]) {
void checkRuleOf3(const char code[]) {
// Clear the error log
errout.str("");
Settings settings;
@ -207,60 +207,53 @@ private:
// Check..
CheckClass checkClass(&tokenizer, &settings, this);
checkClass.checkCopyCtorAndEqOperator();
checkClass.checkRuleOf3();
}
void copyCtorAndEqOperator() {
checkCopyCtorAndEqOperator("class A \n"
"{ \n"
void ruleOf3() {
checkRuleOf3("class A {\n"
" A(const A& other) { } \n"
" A& operator=(const A& other) { return *this; }\n"
"};");
ASSERT_EQUALS("", errout.str());
checkCopyCtorAndEqOperator("class A \n"
"{ \n"
"};");
checkRuleOf3("class A {};");
ASSERT_EQUALS("", errout.str());
checkCopyCtorAndEqOperator("class A \n"
"{ \n"
checkRuleOf3("class A {\n"
" A(const A& other) { } \n"
"};");
ASSERT_EQUALS("", errout.str());
checkCopyCtorAndEqOperator("class A \n"
"{ \n"
checkRuleOf3("class A {\n"
" A& operator=(const A& other) { return *this; }\n"
"};");
ASSERT_EQUALS("", errout.str());
checkCopyCtorAndEqOperator("class A \n"
"{ \n"
checkRuleOf3("class A {\n"
" A(const A& other) { } \n"
" int x;\n"
"};");
ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' has 'copy constructor' but lack of 'operator='.\n", errout.str());
ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' defines 'copy constructor' but does not define 'operator=' and 'destructor'\n", errout.str());
checkCopyCtorAndEqOperator("class A \n"
"{ \n"
checkRuleOf3("class A {\n"
" A& operator=(const A& other) { return *this; }\n"
" int x;\n"
"};");
ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' has 'operator=' but lack of 'copy constructor'.\n", errout.str());
ASSERT_EQUALS("[test.cpp:1]: (warning) The class 'A' defines 'operator=' but does not define 'copy constructor' and 'destructor'\n", errout.str());
checkCopyCtorAndEqOperator("class A \n"
"{ \n"
checkRuleOf3("class A {\n"
" A& operator=(const int &x) { this->x = x; return *this; }\n"
" int x;\n"
"};");
ASSERT_EQUALS("", errout.str());
checkCopyCtorAndEqOperator("class A {\n"
checkRuleOf3("class A {\n"
"public:\n"
" A() : x(0) { }\n"
" ~A() {}\n"
" A(const A & a) { x = a.x; }\n"
" A & operator = (const A & a) {\n"
" x = a.x;\n"
@ -279,7 +272,7 @@ private:
ASSERT_EQUALS("", errout.str());
// #7987 - Don't show warning when there is a move constructor
checkCopyCtorAndEqOperator("struct S {\n"
checkRuleOf3("struct S {\n"
" std::string test;\n"
" S(S&& s) : test(std::move(s.test)) { }\n"
" S& operator = (S &&s) {\n"
@ -290,7 +283,7 @@ private:
ASSERT_EQUALS("", errout.str());
// #8337 - False positive in copy constructor detection
checkCopyCtorAndEqOperator("struct StaticListNode {\n"
checkRuleOf3("struct StaticListNode {\n"
" StaticListNode(StaticListNode*& prev) : m_next(0) {}\n"
" StaticListNode* m_next;\n"
"};");