diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 54827b067..3ebbc3b28 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -153,98 +153,6 @@ void CheckClass::constructors() } } -void CheckClass::copyconstructors() -{ - if (!_settings->isEnabled("style")) - return; - - for (std::list::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - if (!scope->isClassOrStruct()) // scope is class or structure - continue; - - std::map allocatedVars; - - for (std::list::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - if (func->type == Function::eConstructor && func->functionScope) { - for (const Token* tok = func->functionScope->classStart; tok!=func->functionScope->classEnd; tok=tok->next()) { - if (Token::Match(tok, "%var% = new|malloc|g_malloc|g_try_malloc|realloc|g_realloc|g_try_realloc")) { - const Variable* var = symbolDatabase->getVariableFromVarId(tok->varId()); - if (var && var->isPointer() && var->scope() == &*scope) - allocatedVars[tok->varId()] = tok; - } - } - } - } - - std::set copiedVars; - const Token* copyCtor = 0; - for (std::list::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - if (func->type == Function::eCopyConstructor) { - copyCtor = func->tokenDef; - if (func->functionScope) { - const Token* tok = func->tokenDef->linkAt(1)->next(); - if (tok->str()==":") { - tok=tok->next(); - while (Token::Match(tok, "%var% (")) { - if (allocatedVars.find(tok->varId()) != allocatedVars.end()) { - if (tok->varId() && Token::Match(tok->tokAt(2), "%var% . %var% )")) - copiedVars.insert(tok); - else if (!Token::Match(tok->tokAt(2), "%any% )")) - allocatedVars.erase(tok->varId()); // Assume memory is allocated - } - tok = tok->linkAt(1)->tokAt(2); - } - } - for (tok=func->functionScope->classStart; tok!=func->functionScope->classEnd; tok=tok->next()) { - if (Token::Match(tok, "%var% = new|malloc|g_malloc|g_try_malloc|realloc|g_realloc|g_try_realloc")) { - allocatedVars.erase(tok->varId()); - } else if (Token::Match(tok, "%var% = %var% . %var% ;") && allocatedVars.find(tok->varId()) != allocatedVars.end()) { - copiedVars.insert(tok); - } - } - } else // non-copyable or implementation not seen - allocatedVars.clear(); - break; - } - } - if (!copyCtor) { - if (!allocatedVars.empty() && scope->derivedFrom.empty()) // TODO: Check if base class is non-copyable - noCopyConstructorError(scope->classDef, scope->className, scope->type == Scope::eStruct); - } else { - if (!copiedVars.empty()) { - for (std::set::const_iterator i = copiedVars.begin(); i != copiedVars.end(); ++i) { - copyConstructorShallowCopyError(*i, (*i)->str()); - } - } - // throw error if count mismatch - for (std::map::const_iterator i = allocatedVars.begin(); i != allocatedVars.end(); ++i) { - copyConstructorMallocError(copyCtor, i->second, i->second->str()); - } - } - } -} - -void CheckClass::copyConstructorMallocError(const Token *cctor, const Token *alloc, const std::string& varname) -{ - std::list callstack; - callstack.push_back(cctor); - callstack.push_back(alloc); - reportError(callstack, Severity::warning, "copyCtorNoAllocation", "Copy constructor does not allocate memory for member '" + varname + "' although memory has been allocated in other constructors."); -} - -void CheckClass::copyConstructorShallowCopyError(const Token *tok, const std::string& varname) -{ - reportError(tok, Severity::style, "copyCtorPointerCopying", "Value of pointer '" + varname + "', which points to allocated memory, is copied in copy constructor instead of allocating new memory."); -} - -void CheckClass::noCopyConstructorError(const Token *tok, const std::string &classname, bool isStruct) -{ - // The constructor might be intentionally missing. Therefore this is not a "warning" - reportError(tok, Severity::style, "noCopyConstructor", - "'" + std::string(isStruct ? "struct" : "class") + " " + classname + - "' does not have a copy constructor which is required since the class contains a pointer to allocated memory."); -} - bool CheckClass::canNotCopy(const Scope *scope) { std::list::const_iterator func; @@ -827,7 +735,7 @@ void CheckClass::checkMemsetType(const Scope *start, const Token *tok, const Sco std::list::const_iterator var; for (var = type->varlist.begin(); var != type->varlist.end(); ++var) { - // don't warn if variable static or const, pointer or reference + // don't warn if variable static or const, pointer or referece if (!var->isStatic() && !var->isConst() && !var->isPointer() && !var->isReference()) { const Token *tok1 = var->typeStartToken(); @@ -1263,7 +1171,7 @@ void CheckClass::thisSubtractionError(const Token *tok) void CheckClass::checkConst() { - // This is an inconclusive check. False positives: #3322. + // This is an inconclusive check. False positives: #2340, #3322. if (!_settings->inconclusive) return; diff --git a/lib/checkclass.h b/lib/checkclass.h index 2a9da018a..082a0cc14 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -72,7 +72,6 @@ public: checkClass.virtualDestructor(); checkClass.checkConst(); - checkClass.copyconstructors(); } @@ -116,16 +115,11 @@ public: void initializationListUsage(); - void copyconstructors(); - private: const SymbolDatabase *symbolDatabase; // Reporting errors.. void noConstructorError(const Token *tok, const std::string &classname, bool isStruct); - void copyConstructorMallocError(const Token *cctor, const Token *alloc, const std::string& var_name); - void copyConstructorShallowCopyError(const Token *tok, const std::string& varname); - void noCopyConstructorError(const Token *tok, const std::string &classname, bool isStruct); void uninitVarError(const Token *tok, const std::string &classname, const std::string &varname); void operatorEqVarError(const Token *tok, const std::string &classname, const std::string &varname); void unusedPrivateFunctionError(const Token *tok, const std::string &classname, const std::string &funcname); @@ -143,9 +137,6 @@ private: void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckClass c(0, settings, errorLogger); c.noConstructorError(0, "classname", false); - c.copyConstructorMallocError(0, 0, "var"); - c.copyConstructorShallowCopyError(0, "var"); - c.noCopyConstructorError(0, "class", false); c.uninitVarError(0, "classname", "varname"); c.operatorEqVarError(0, "classname", ""); c.unusedPrivateFunctionError(0, "classname", "funcname"); @@ -167,8 +158,7 @@ private: std::string classInfo() const { return "Check the code for each class.\n" - "* Missing constructors and copy constructors\n" - "* Missing allocation of memory in copy constructor\n" + "* Missing constructors\n" "* Are all variables initialized by the constructors?\n" "* Are all variables assigned by 'operator='?\n" "* Warn if memset, memcpy etc are used on a class\n" diff --git a/test/testclass.cpp b/test/testclass.cpp index f1e0f359c..23e8d72e1 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -43,8 +43,6 @@ private: TEST_CASE(virtualDestructorInherited); TEST_CASE(virtualDestructorTemplate); - TEST_CASE(copyConstructor); - TEST_CASE(noConstructor1); TEST_CASE(noConstructor2); TEST_CASE(noConstructor3); @@ -163,215 +161,6 @@ private: TEST_CASE(initializerListUsage); } - void checkCopyConstructor(const char code[]) { - // Clear the error log - errout.str(""); - Settings settings; - settings.addEnabled("style"); - - // Tokenize.. - Tokenizer tokenizer(&settings, this); - std::istringstream istr(code); - tokenizer.tokenize(istr, "test.cpp"); - tokenizer.simplifyTokenList(); - - // Check.. - CheckClass checkClass(&tokenizer, &settings, this); - checkClass.copyconstructors(); - } - - void copyConstructor() { - checkCopyConstructor("class F\n" - "{\n" - " public:\n" - " char *c,*p,*d;\n" - " F(const F &f) : p(f.p), c(f.c)\n" - " {\n" - " p=(char *)malloc(strlen(f.p)+1);\n" - " strcpy(p,f.p);\n" - " }\n" - " F(char *str)\n" - " {\n" - " p=(char *)malloc(strlen(str)+1);\n" - " strcpy(p,str);\n" - " }\n" - "};"); - ASSERT_EQUALS("[test.cpp:5]: (style) Value of pointer 'p', which points to allocated memory, is copied in copy constructor instead of allocating new memory.\n", errout.str()); - - checkCopyConstructor("class F {\n" - " char *p;\n" - " F(const F &f) {\n" - " p = f.p;\n" - " }\n" - " F(char *str) {\n" - " p = malloc(strlen(str)+1);\n" - " }\n" - "};"); - ASSERT_EQUALS("[test.cpp:4]: (style) Value of pointer 'p', which points to allocated memory, is copied in copy constructor instead of allocating new memory.\n" - "[test.cpp:3] -> [test.cpp:7]: (warning) Copy constructor does not allocate memory for member 'p' although memory has been allocated in other constructors.\n", errout.str()); - - checkCopyConstructor("class F\n" - "{\n" - " public:\n" - " char *c,*p,*d;\n" - " F(const F &f) :p(f.p)\n" - " {\n" - " }\n" - " F(char *str)\n" - " {\n" - " p=(char *)malloc(strlen(str)+1);\n" - " strcpy(p,str);\n" - " }\n" - "};"); - ASSERT_EQUALS("[test.cpp:5]: (style) Value of pointer 'p', which points to allocated memory, is copied in copy constructor instead of allocating new memory.\n" - "[test.cpp:5] -> [test.cpp:10]: (warning) Copy constructor does not allocate memory for member 'p' although memory has been allocated in other constructors.\n", errout.str()); - - checkCopyConstructor("class kalci\n" - "{\n" - " public:\n" - " char *c,*p,*d;\n" - " kalci()\n" - " {\n" - " p=(char *)malloc(100);\n" - " strcpy(p,\"hello\");\n" - " c=(char *)malloc(100);\n" - " strcpy(p,\"hello\");\n" - " d=(char *)malloc(100);\n" - " strcpy(p,\"hello\");\n" - " }\n" - " kalci(const kalci &f)\n" - " {\n" - " p=(char *)malloc(strlen(str)+1);\n" - " strcpy(p,f.p);\n" - " c=(char *)malloc(strlen(str)+1);\n" - " strcpy(p,f.p);\n" - " d=(char *)malloc(strlen(str)+1);\n" - " strcpy(p,f.p);\n" - " }\n" - "};"); - ASSERT_EQUALS("", errout.str()); - - checkCopyConstructor("class F\n" - "{\n" - " public:\n" - " char *c,*p,*d;\n" - " F(char *str,char *st,char *string)\n" - " {\n" - " p=(char *)malloc(100);\n" - " strcpy(p,str);\n" - " c=(char *)malloc(100);\n" - " strcpy(p,st);\n" - " d=(char *)malloc(100);\n" - " strcpy(p,string);\n" - " }\n" - " F(const F &f)\n" - " {\n" - " p=(char *)malloc(strlen(str)+1);\n" - " strcpy(p,f.p);\n" - " c=(char *)malloc(strlen(str)+1);\n" - " strcpy(p,f.p);\n" - " }\n" - "};"); - ASSERT_EQUALS("[test.cpp:14] -> [test.cpp:11]: (warning) Copy constructor does not allocate memory for member 'd' although memory has been allocated in other constructors.\n", errout.str()); - - checkCopyConstructor("class F {\n" - " char *c;\n" - " F(char *str,char *st,char *string) {\n" - " p=(char *)malloc(100);\n" - " }\n" - " F(const F &f)\n" - " : p(malloc(size))\n" - " {\n" - " }\n" - "};"); - ASSERT_EQUALS("", errout.str()); - - checkCopyConstructor("class F {\n" - " char *c;\n" - " F(char *str,char *st,char *string)\n" - " : p(malloc(size))\n" - " {\n" - " }\n" - " F(const F &f)\n" - " {\n" - " }\n" - "};"); - TODO_ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:4]: (warning) Copy constructor does not allocate memory for member 'd' although memory has been allocated in other constructors.\n", "", errout.str()); - - checkCopyConstructor("class F\n" - "{\n" - " public:\n" - " char *c,*p,*d;\n" - " F()\n" - " {\n" - " p=(char *)malloc(100);\n" - " c=(char *)malloc(100);\n" - " d=(char*)malloc(100);\n" - " }\n" - "};"); - ASSERT_EQUALS("[test.cpp:1]: (style) 'class F' does not have a copy constructor which is required since the class contains a pointer to allocated memory.\n", errout.str()); - - checkCopyConstructor("class F\n" - "{\n" - " public:\n" - " char *c;\n" - " const char *p,*d;\n" - " F(char *str,char *st,char *string)\n" - " {\n" - " p=str;\n" - " d=st;\n" - " c=(char *)malloc(strlen(string)+1);\n" - " strcpy(d,string);\n" - " }\n" - " F(const F &f)\n" - " {\n" - " p=f.p;\n" - " d=f.d;\n" - " c=(char *)malloc(strlen(str)+1);\n" - " strcpy(d,f.p);\n" - " }\n" - "};"); - ASSERT_EQUALS("", errout.str()); - - checkCopyConstructor("class F : E\n" - "{\n" - " char *p;\n" - " F() {\n" - " p = malloc(100);\n" - " }\n" - "};"); - ASSERT_EQUALS("", errout.str()); - - checkCopyConstructor("class E { E(E&); };\n" // non-copyable - "class F : E\n" - "{\n" - " char *p;\n" - " F() {\n" - " p = malloc(100);\n" - " }\n" - "};"); - ASSERT_EQUALS("", errout.str()); - - checkCopyConstructor("class E {};\n" - "class F : E {\n" - " char *p;\n" - " F() {\n" - " p = malloc(100);\n" - " }\n" - "};"); - TODO_ASSERT_EQUALS("[test.cpp:2]: (style) 'class F' does not have a copy constructor which is required since the class contains a pointer to allocated memory.\n", "", errout.str()); - - checkCopyConstructor("class F {\n" - " char *p;\n" - " F() {\n" - " p = malloc(100);\n" - " }\n" - " F(F& f);\n" // non-copyable - "};"); - ASSERT_EQUALS("", errout.str()); - } - - // Check the operator Equal void checkOpertorEq(const char code[]) { // Clear the error log