From 54e7c8f6a25dc16cd641a801446f16915fa5d6ea Mon Sep 17 00:00:00 2001 From: Frank Zingsheim Date: Wed, 10 Apr 2013 21:57:22 +0200 Subject: [PATCH] Implemented support for move constructors: Adapt code to Function::eMoveConstructor introduced in commit eb2962792f50e440dc126b0e9c548623e78d3d4b --- lib/checkclass.cpp | 64 +++++++++++++++++++-------- lib/checkclass.h | 2 + lib/checkmemoryleak.cpp | 4 +- lib/checknullpointer.cpp | 2 +- lib/symboldatabase.cpp | 13 +++--- lib/symboldatabase.h | 12 +++++- test/testclass.cpp | 14 ++++++ test/testconstructors.cpp | 91 ++++++++++++++++++++++++++++++++++++--- 8 files changed, 169 insertions(+), 33 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 71a548799..c2a04f94c 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -120,8 +120,7 @@ void CheckClass::constructors() std::vector usage(scope->varlist.size()); for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - if (!func->hasBody || !(func->type == Function::eConstructor || - func->type == Function::eCopyConstructor || + if (!func->hasBody || !(func->isConstructor() || func->type == Function::eOperatorEqual)) continue; @@ -160,8 +159,15 @@ void CheckClass::constructors() } // Check if type can't be copied - if (!var->isPointer() && var->typeScope() && canNotCopy(var->typeScope())) - continue; + if (!var->isPointer() && var->typeScope()) { + if (func->type == Function::eMoveConstructor) { + if (canNotMove(var->typeScope())) + continue; + } else { + if (canNotCopy(var->typeScope())) + continue; + } + } // Don't warn about unknown types in copy constructors since we // don't know if they can be copied or not.. @@ -197,7 +203,7 @@ void CheckClass::constructors() const Scope *varType = var->typeScope(); if (!varType || varType->type != Scope::eUnion) { if (func->type == Function::eConstructor && - func->nestedIn && (func->nestedIn->numConstructors - func->nestedIn->numCopyConstructors) > 1 && + func->nestedIn && (func->nestedIn->numConstructors - func->nestedIn->numCopyOrMoveConstructors) > 1 && func->argCount() == 0 && func->functionScope && func->arg && func->arg->link()->next() == func->functionScope->classStart && func->functionScope->classStart->link() == func->functionScope->classStart->next()) { @@ -324,9 +330,10 @@ bool CheckClass::canNotCopy(const Scope *scope) bool publicCopy = false; for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - if (func->type == Function::eConstructor || func->type == Function::eCopyConstructor) + if (func->isConstructor()) constructor = true; - if (func->type == Function::eCopyConstructor && func->access == Public) + if ((func->type == Function::eCopyConstructor) && + func->access == Public) publicCopy = true; else if (func->type == Function::eOperatorEqual && func->access == Public) publicAssign = true; @@ -335,6 +342,30 @@ bool CheckClass::canNotCopy(const Scope *scope) return constructor && !(publicAssign || publicCopy); } +bool CheckClass::canNotMove(const Scope *scope) +{ + std::list::const_iterator func; + bool constructor = false; + bool publicAssign = false; + bool publicCopy = false; + bool publicMove = false; + + for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { + if (func->isConstructor()) + constructor = true; + if ((func->type == Function::eCopyConstructor) && + func->access == Public) + publicCopy = true; + else if ((func->type == Function::eMoveConstructor) && + func->access == Public) + publicMove = true; + else if (func->type == Function::eOperatorEqual && func->access == Public) + publicAssign = true; + } + + return constructor && !(publicAssign || publicCopy || publicMove); +} + void CheckClass::assignVar(const std::string &varname, const Scope *scope, std::vector &usage) { std::list::const_iterator var; @@ -401,7 +432,7 @@ bool CheckClass::isBaseClassFunc(const Token *tok, const Scope *scope) void CheckClass::initializeVarList(const Function &func, std::list &callstack, const Scope *scope, std::vector &usage) { - bool initList = func.type == Function::eConstructor || func.type == Function::eCopyConstructor; + bool initList = func.isConstructor(); const Token *ftok = func.arg->link()->next(); int level = 0; for (; ftok != func.functionScope->classEnd; ftok = ftok->next()) { @@ -582,7 +613,7 @@ void CheckClass::initializeVarList(const Function &func, std::listfunction() && ftok->function()->nestedIn == scope && - ftok->function()->type != Function::eConstructor) { + !ftok->function()->isConstructor()) { const Function *member = ftok->function(); // recursive call @@ -621,7 +652,7 @@ void CheckClass::initializeVarList(const Function &func, std::listfunctionScopes[i]; // Check every constructor - if (!scope->function || (scope->function->type != Function::eConstructor && scope->function->type != Function::eCopyConstructor)) + if (!scope->function || (!scope->function->isConstructor())) continue; const Scope* owner = scope->functionOf; @@ -728,7 +759,8 @@ void CheckClass::initializationListUsage() for (const Token* tok2 = tok->tokAt(2); tok2->str() != ";"; tok2 = tok2->next()) { if (tok2->varId()) { const Variable* var2 = tok2->variable(); - if (var2 && var2->scope() == owner) { // Is there a dependency between two member variables? + if (var2 && var2->scope() == owner && + tok2->strAt(-1)!=".") { // Is there a dependency between two member variables? allowed = false; break; } @@ -1816,7 +1848,7 @@ void CheckClass::initializerListOrder() // iterate through all member functions looking for constructors for (func = info->functionList.begin(); func != info->functionList.end(); ++func) { - if ((func->type == Function::eConstructor || func->type == Function::eCopyConstructor) && func->hasBody) { + if ((func->isConstructor()) && func->hasBody) { // check for initializer list const Token *tok = func->arg->link()->next(); @@ -1878,10 +1910,8 @@ void CheckClass::checkPureVirtualFunctionCall() for (std::size_t i = 0; i < functions; ++i) { const Scope * scope = symbolDatabase->functionScopes[i]; if (scope->function == 0 || !scope->function->hasBody || - !(scope->function->type==Function::eConstructor || - scope->function->type==Function::eCopyConstructor || - scope->function->type==Function::eMoveConstructor || - scope->function->type==Function::eDestructor)) + !(scope->function->isConstructor() || + scope->function->isDestructor())) continue; const std::list & pureVirtualFunctionCalls=callsPureVirtualFunction(*scope->function,callsPureVirtualFunctionMap); diff --git a/lib/checkclass.h b/lib/checkclass.h index cf7098b09..966d31274 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -279,6 +279,8 @@ private: std::list & pureFuncStack); static bool canNotCopy(const Scope *scope); + + static bool canNotMove(const Scope *scope); }; /// @} //--------------------------------------------------------------------------- diff --git a/lib/checkmemoryleak.cpp b/lib/checkmemoryleak.cpp index 514149522..7c4fb93e5 100644 --- a/lib/checkmemoryleak.cpp +++ b/lib/checkmemoryleak.cpp @@ -2368,8 +2368,8 @@ void CheckMemoryLeakInClass::variable(const Scope *scope, const Token *tokVarnam // Inspect member functions std::list::const_iterator func; for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - const bool constructor = func->type == Function::eConstructor; - const bool destructor = func->type == Function::eDestructor; + const bool constructor = func->isConstructor(); + const bool destructor = func->isDestructor(); if (!func->hasBody) { if (destructor) { // implementation for destructor is not seen => assume it deallocates all variables properly deallocInDestructor = true; diff --git a/lib/checknullpointer.cpp b/lib/checknullpointer.cpp index 37d64fe67..a1e5a9aa2 100644 --- a/lib/checknullpointer.cpp +++ b/lib/checknullpointer.cpp @@ -1131,7 +1131,7 @@ void CheckNullPointer::nullConstantDereference() const Token *tok = scope->classStart; - if (scope->function && (scope->function->type == Function::eConstructor || scope->function->type == Function::eCopyConstructor)) + if (scope->function && scope->function->isConstructor()) tok = scope->function->token; // Check initialization list for (; tok != scope->classEnd; tok = tok->next()) { diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 79eeb3e7c..cb48a07bf 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -433,12 +433,11 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti function.isConst = true; // count the number of constructors - if (function.type == Function::eConstructor) + if (function.isConstructor()) scope->numConstructors++; - else if (function.type == Function::eCopyConstructor) { - scope->numConstructors++; - scope->numCopyConstructors++; - } + if (function.type == Function::eCopyConstructor || + function.type == Function::eMoveConstructor) + scope->numCopyOrMoveConstructors++; // assume implementation is inline (definition and implementation same) function.token = function.tokenDef; @@ -1987,7 +1986,7 @@ Scope::Scope(const SymbolDatabase *check_, const Token *classDef_, const Scope * classEnd(start_->link()), nestedIn(nestedIn_), numConstructors(0), - numCopyConstructors(0), + numCopyOrMoveConstructors(0), type(type_), definedType(NULL), functionOf(NULL), @@ -2002,7 +2001,7 @@ Scope::Scope(const SymbolDatabase *check_, const Token *classDef_, const Scope * classEnd(NULL), nestedIn(nestedIn_), numConstructors(0), - numCopyConstructors(0), + numCopyOrMoveConstructors(0), definedType(NULL), functionOf(NULL), function(NULL) diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 6af8bc6c8..0d7ed461f 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -476,6 +476,16 @@ public: /** @brief check if this function is virtual in the base classes */ bool isImplicitlyVirtual(bool defaultVal = false) const; + bool isConstructor() const { + return type==eConstructor || + type==eCopyConstructor || + type==eMoveConstructor; + } + + bool isDestructor() const { + return type==eDestructor; + } + const Token *tokenDef; // function name token in class definition const Token *argDef; // function argument start '(' in class definition const Token *token; // function name token in implementation @@ -530,7 +540,7 @@ public: const Scope *nestedIn; std::list nestedList; unsigned int numConstructors; - unsigned int numCopyConstructors; + unsigned int numCopyOrMoveConstructors; std::list usingList; ScopeType type; Type* definedType; diff --git a/test/testclass.cpp b/test/testclass.cpp index 8f662037e..407aa0316 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -5544,6 +5544,20 @@ private: "};"); ASSERT_EQUALS("[test.cpp:4]: (performance) Variable 'c' is assigned in constructor body. Consider performing initialization in initialization list.\n", errout.str()); + checkInitializationListUsage("class C;\n" + "class Fred {\n" + " C c;\n" + " Fred(Fred const & other) { c = other.c; }\n" + "};"); + ASSERT_EQUALS("[test.cpp:4]: (performance) Variable 'c' is assigned in constructor body. Consider performing initialization in initialization list.\n", errout.str()); + + checkInitializationListUsage("class C;\n" + "class Fred {\n" + " C c;\n" + " Fred(Fred && other) { c = other.c; }\n" + "};"); + ASSERT_EQUALS("[test.cpp:4]: (performance) Variable 'c' is assigned in constructor body. Consider performing initialization in initialization list.\n", errout.str()); + checkInitializationListUsage("class C;\n" "class Fred {\n" " C a;\n" diff --git a/test/testconstructors.cpp b/test/testconstructors.cpp index 189d44301..094cd31f3 100644 --- a/test/testconstructors.cpp +++ b/test/testconstructors.cpp @@ -190,6 +190,8 @@ private: "{\n" "public:\n" " Fred() : i(0) { }\n" + " Fred(Fred const & other) : i(other.i) {}\n" + " Fred(Fred && other) : i(other.i) {}\n" " int i;\n" "};"); ASSERT_EQUALS("", errout.str()); @@ -198,6 +200,8 @@ private: "{\n" "public:\n" " Fred() { i = 0; }\n" + " Fred(Fred const & other) {i=other.i}\n" + " Fred(Fred && other) {i=other.i}\n" " int i;\n" "};"); ASSERT_EQUALS("", errout.str()); @@ -206,16 +210,24 @@ private: "{\n" "public:\n" " Fred() { }\n" + " Fred(Fred const & other) {}\n" + " Fred(Fred && other) {}\n" " int i;\n" "};"); - ASSERT_EQUALS("[test.cpp:4]: (warning) Member variable 'Fred::i' is not initialized in the constructor.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) Member variable 'Fred::i' is not initialized in the constructor.\n" + "[test.cpp:5]: (warning) Member variable 'Fred::i' is not initialized in the constructor.\n" + "[test.cpp:6]: (warning) Member variable 'Fred::i' is not initialized in the constructor.\n", errout.str()); check("struct Fred\n" "{\n" " Fred() { }\n" + " Fred(Fred const & other) {}\n" + " Fred(Fred && other) {}\n" " int i;\n" "};"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Member variable 'Fred::i' is not initialized in the constructor.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Member variable 'Fred::i' is not initialized in the constructor.\n" + "[test.cpp:4]: (warning) Member variable 'Fred::i' is not initialized in the constructor.\n" + "[test.cpp:5]: (warning) Member variable 'Fred::i' is not initialized in the constructor.\n", errout.str()); } @@ -254,6 +266,7 @@ private: "{\n" " Fred();\n" " explicit Fred(int _i);\n" + " Fred(Fred const & other);\n" " int i;\n" "};\n" "Fred::Fred()\n" @@ -262,7 +275,7 @@ private: "{\n" " i = _i;\n" "}\n", true); - ASSERT_EQUALS("[test.cpp:7]: (warning, inconclusive) Member variable 'Fred::i' is not initialized in the constructor.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8]: (warning, inconclusive) Member variable 'Fred::i' is not initialized in the constructor.\n", errout.str()); } void simple5() { // ticket #2560 @@ -1004,7 +1017,6 @@ private: check("class B\n" "{\n" " B (const B & Var);\n" - " B & operator= (const B & Var);\n" "};\n" "class A\n" "{\n" @@ -1012,6 +1024,39 @@ private: "public:\n" " A(){}\n" " A(const A&){}\n" + " A(A &&){}\n" + " const A& operator=(const A&){return *this;}\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + check("class B\n" + "{\n" + " B (B && Var);\n" + "};\n" + "class A\n" + "{\n" + " B m_SemVar;\n" + "public:\n" + " A(){}\n" + " A(const A&){}\n" + " A(A &&){}\n" + " const A& operator=(const A&){return *this;}\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + check("class B\n" + "{\n" + " B & operator= (const B & Var);\n" + "public:\n" + " B ();\n" + "};\n" + "class A\n" + "{\n" + " B m_SemVar;\n" + "public:\n" + " A(){}\n" + " A(const A&){}\n" + " A(A &&){}\n" " const A& operator=(const A&){return *this;}\n" "};"); ASSERT_EQUALS("", errout.str()); @@ -1020,6 +1065,40 @@ private: "{\n" "public:\n" " B (const B & Var);\n" + "};\n" + "class A\n" + "{\n" + " B m_SemVar;\n" + "public:\n" + " A(){}\n" + " A(const A&){}\n" + " A(A &&){}\n" + " const A& operator=(const A&){return *this;}\n" + "};"); + ASSERT_EQUALS("[test.cpp:11]: (warning) Member variable 'A::m_SemVar' is not initialized in the constructor.\n" + "[test.cpp:12]: (warning) Member variable 'A::m_SemVar' is not initialized in the constructor.\n" + "[test.cpp:13]: (warning) Member variable 'A::m_SemVar' is not assigned a value in 'A::operator='.\n", errout.str()); + + check("class B\n" + "{\n" + "public:\n" + " B (B && Var);\n" + "};\n" + "class A\n" + "{\n" + " B m_SemVar;\n" + "public:\n" + " A(){}\n" + " A(const A&){}\n" + " A(A &&){}\n" + " const A& operator=(const A&){return *this;}\n" + "};"); + ASSERT_EQUALS("[test.cpp:12]: (warning) Member variable 'A::m_SemVar' is not initialized in the constructor.\n", errout.str()); + + check("class B\n" + "{\n" + "public:\n" + " B ();\n" " B & operator= (const B & Var);\n" "};\n" "class A\n" @@ -1028,10 +1107,12 @@ private: "public:\n" " A(){}\n" " A(const A&){}\n" + " A(A &&){}\n" " const A& operator=(const A&){return *this;}\n" "};"); ASSERT_EQUALS("[test.cpp:12]: (warning) Member variable 'A::m_SemVar' is not initialized in the constructor.\n" - "[test.cpp:13]: (warning) Member variable 'A::m_SemVar' is not assigned a value in 'A::operator='.\n", errout.str()); + "[test.cpp:13]: (warning) Member variable 'A::m_SemVar' is not initialized in the constructor.\n" + "[test.cpp:14]: (warning) Member variable 'A::m_SemVar' is not assigned a value in 'A::operator='.\n", errout.str()); check("class A\n" "{\n"