Revert "CheckClass::copyconstructors: Removed check. Because there is unfixed ticket #4154."

This reverts commit 066a1d48fe.

lib/checkclass.cpp

@@ -153,6 +153,98 @@ void CheckClass::constructors()
     }
 }
 
+void CheckClass::copyconstructors()
+{
+    if (!_settings->isEnabled("style"))
+        return;
+
+    for (std::list<Scope>::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
+        if (!scope->isClassOrStruct()) // scope is class or structure
+            continue;
+
+        std::map<unsigned int, const Token*> allocatedVars;
+
+        for (std::list<Function>::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<const Token*> copiedVars;
+        const Token* copyCtor = 0;
+        for (std::list<Function>::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 Token*>::const_iterator i = copiedVars.begin(); i != copiedVars.end(); ++i) {
+                    copyConstructorShallowCopyError(*i, (*i)->str());
+                }
+            }
+            // throw error if count mismatch
+            for (std::map<unsigned int, const Token*>::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<const Token*> 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<Function>::const_iterator func;
@@ -735,7 +827,7 @@ void CheckClass::checkMemsetType(const Scope *start, const Token *tok, const Sco
     std::list<Variable>::const_iterator var;
 
     for (var = type->varlist.begin(); var != type->varlist.end(); ++var) {
-        // don't warn if variable static or const, pointer or referece
+        // don't warn if variable static or const, pointer or reference
         if (!var->isStatic() && !var->isConst() && !var->isPointer() && !var->isReference()) {
             const Token *tok1 = var->typeStartToken();
 
@@ -1171,7 +1263,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;
 

lib/checkclass.h

@@ -72,6 +72,7 @@ public:
 
         checkClass.virtualDestructor();
         checkClass.checkConst();
+        checkClass.copyconstructors();
     }
 
 
@@ -115,11 +116,16 @@ 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);
@@ -137,6 +143,9 @@ 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");
@@ -158,7 +167,8 @@ private:
 
     std::string classInfo() const {
         return "Check the code for each class.\n"
-               "* Missing constructors\n"
+               "* Missing constructors and copy constructors\n"
+               "* Missing allocation of memory in copy constructor\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"

test/testclass.cpp

@@ -43,6 +43,8 @@ private:
         TEST_CASE(virtualDestructorInherited);
         TEST_CASE(virtualDestructorTemplate);
 
+        TEST_CASE(copyConstructor);
+
         TEST_CASE(noConstructor1);
         TEST_CASE(noConstructor2);
         TEST_CASE(noConstructor3);
@@ -161,6 +163,215 @@ 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