diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 751bec4c1..4bf1f111f 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -157,125 +157,92 @@ void CheckClass::copyconstructors() { if (!_settings->isEnabled("style")) return; - std::vector var_id; - std::vector var_name; - std::list::const_iterator scope; - std::list::const_iterator func; - unsigned int flag=0,varid=0; - for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - if (!scope->isClassOrStruct()) /*scope is class or structure*/ + + for (std::list::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { + if (!scope->isClassOrStruct()) // scope is class or structure continue; - int count_no_variables=0,count_copy_constructor=0,count_no_allocated_variables=0,count_no_of_pointer_variable=0; - std::list::const_iterator var; - for (var = scope->varlist.begin(); var != scope->varlist.end(); ++var) { - if (var->isPointer()) { - count_no_of_pointer_variable ++; - } - } - if (count_no_of_pointer_variable>0) { - for (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()) { - const char pattern[] = "%var% = new|malloc|g_malloc|g_try_malloc|realloc|g_realloc|g_try_realloc"; - if (Token::Match(tok, pattern)) { - std::vector::iterator it=var_name.begin(); - std::vector::iterator itr; - for (itr=var_id.begin(); itr!=var_id.end(); ++itr,++it) { - if (*itr==tok->varId()) { - break; - } - } - if (itr==var_id.end()) { - var_id.push_back(tok->varId()); - var_name.push_back(tok->str()); - count_no_allocated_variables++; - } - } + + 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; } } } } - for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { + + 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) { - count_copy_constructor++; - break; - } - } - if (!var_id.empty() && count_copy_constructor==0) { - noCopyConstructorError(scope->classDef, scope->className, scope->classDef->str() == "struct"); - } - if (count_copy_constructor==1) { - for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - if (func->type == Function::eCopyConstructor && func->functionScope) { + copyCtor = func->tokenDef; + if (func->functionScope) { const Token* tok = func->tokenDef->linkAt(1)->next(); if (tok->str()==":") { tok=tok->next(); - if (Token::Match(tok,"%var% ( %var% . %var% )")) { - - for (std::vector::iterator itr=var_id.begin(); itr!=var_id.end(); ++itr) { - if (*itr==tok->varId()) { - flag=1; - varid=tok->varId(); - - } + 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()) { - const char pattern[] = "%var% = new|malloc|g_malloc|g_try_malloc|realloc|g_realloc|g_try_realloc"; - if (Token::Match(tok, pattern)) { - std::vector::iterator it=var_name.begin(); - - for (std::vector::iterator itr=var_id.begin(); itr!=var_id.end(); ++itr,++it) { - if (*itr==tok->varId()) { - if (varid==tok->varId()) { - flag=0; - } - count_no_variables++; - var_id.erase(itr); - var_name.erase(it); - break; - } - } + 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); } } - } - } - if (flag==1) { - copyConstructorShallowCopyError(scope->classDef, scope->className, scope->classDef->str() == "struct"); - } - /*if count mismatch throw error*/ - if (count_no_variables!=count_no_allocated_variables) { - copyConstructorMallocError(scope->classDef, scope->className, scope->classDef->str() == "struct",var_name); + } 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()); } } - var_id.clear(); - var_name.clear(); } } -void CheckClass::copyConstructorMallocError(const Token *tok, const std::string &classname, bool isStruct, const std::vector& var_name) +void CheckClass::copyConstructorMallocError(const Token *cctor, const Token *alloc, const std::string& varname) { - for (std::vector::const_iterator itr=var_name.begin(); itr!=var_name.end(); ++itr) { - reportError(tok, Severity::style, "copyConstructorNoAllocation", "The copy constructor of " + std::string(isStruct ? "struct" : "class") + " '" + classname +"' does not allocate memory for class member " + *itr+"."); - } + 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 &classname, bool isStruct) +void CheckClass::copyConstructorShallowCopyError(const Token *tok, const std::string& varname) { - // For performance reasons the constructor might be intentionally missing. Therefore this is not a "warning" - reportError(tok, Severity::style, "copyConstructorPointerCopying", "The copy constructor of " + std::string(isStruct ? "struct" : "class") + " '" + classname +"' does not allocate memory for pointer class member and copying pointer values."); - + 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) { - // For performance reasons the constructor might be intentionally missing. Therefore this is not a "warning" + // The constructor might be intentionally missing. Therefore this is not a "warning" reportError(tok, Severity::style, "noCopyConstructor", - "The " + std::string(isStruct ? "struct" : "class") + " '" + classname + - "' does not have a copy constructor which is required since the class contains a pointer member."); - + "'" + 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) @@ -1299,7 +1266,7 @@ void CheckClass::thisSubtractionError(const Token *tok) void CheckClass::checkConst() { - // This is an inconclusive check. False positives: #2340, #3322. + // This is an inconclusive check. False positives: #3322. if (!_settings->inconclusive) return; diff --git a/lib/checkclass.h b/lib/checkclass.h index d55fe0b43..6ed588a8f 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -117,8 +117,8 @@ private: // Reporting errors.. void noConstructorError(const Token *tok, const std::string &classname, bool isStruct); - void copyConstructorMallocError(const Token *tok, const std::string &classname, bool isStruct, const std::vector& var_name); - void copyConstructorShallowCopyError(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); @@ -137,9 +137,8 @@ private: void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckClass c(0, settings, errorLogger); c.noConstructorError(0, "classname", false); - std::vector temp; - c.copyConstructorMallocError(0, "class", false, temp); - c.copyConstructorShallowCopyError(0, "class", false); + c.copyConstructorMallocError(0, 0, "var"); + c.copyConstructorShallowCopyError(0, "var"); c.noCopyConstructorError(0, "class", false); c.uninitVarError(0, "classname", "varname"); c.operatorEqVarError(0, "classname", ""); diff --git a/test/testclass.cpp b/test/testclass.cpp index 872b9369d..f1e0f359c 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -185,18 +185,30 @@ private: "{\n" " public:\n" " char *c,*p,*d;\n" - " F(const F &f) :p(f.p)\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(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" + " }\n" "};"); - ASSERT_EQUALS("", errout.str()); + 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" @@ -205,13 +217,14 @@ private: " 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" + " F(char *str)\n" + " {\n" + " p=(char *)malloc(strlen(str)+1);\n" + " strcpy(p,str);\n" + " }\n" "};"); - ASSERT_EQUALS("[test.cpp:1]: (style) The copy constructor of class 'F' does not allocate memory for pointer class member and copying pointer values.\n[test.cpp:1]: (style) The copy constructor of class 'F' does not allocate memory for class member p.\n", errout.str()); + 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" @@ -219,22 +232,22 @@ private: " 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" + " 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" + " p=(char *)malloc(strlen(str)+1);\n" " strcpy(p,f.p);\n" - "}\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()); @@ -242,24 +255,48 @@ private: "{\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" + " 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:1]: (style) The copy constructor of class 'F' does not allocate memory for class member d.\n", errout.str()); + 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" @@ -267,32 +304,69 @@ private: " char *c,*p,*d;\n" " F()\n" " {\n" - " p=(char *)malloc(100);\n" - " c=(char *)malloc(100);\n" - " d=(char*)malloc(100);\n" - " }\n" + " p=(char *)malloc(100);\n" + " c=(char *)malloc(100);\n" + " d=(char*)malloc(100);\n" + " }\n" "};"); - ASSERT_EQUALS("[test.cpp:1]: (style) The class 'F' does not have a copy constructor which is required since the class contains a pointer member.\n", errout.str()); + 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" + " 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" + " 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" - " p=f.p;\n" - " d=f.d;\n" - " c=(char *)malloc(strlen(str)+1);\n" - " strcpy(d,f.p);\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()); }