New check: initialization by itself in initializer list (#3982)

Refactorizations:
- Rearranged code in checkclass.cpp to increase readability
- Several fixes for testclass.cpp tests.

lib/checkclass.cpp

@@ -2039,6 +2039,42 @@ void CheckClass::initializerListError(const Token *tok1, const Token *tok2, cons
                 "initialization errors.", true);
 }
 
+
+//---------------------------------------------------------------------------
+// Check for self initialization in initialization list
+//---------------------------------------------------------------------------
+
+void CheckClass::checkSelfInitialization()
+{
+    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
+    for (std::size_t i = 0; i < symbolDatabase->functionScopes.size(); ++i) {
+        const Scope* scope = symbolDatabase->functionScopes[i];
+        const Function* function = scope->function;
+        if (!function || !function->isConstructor())
+            continue;
+
+        const Token* tok = function->arg->link()->next();
+        if (tok->str() != ":")
+            continue;
+
+        for (; tok != scope->classStart; tok = tok->next()) {
+            if (Token::Match(tok, "[:,] %var% ( %var% )") && tok->next()->varId() && tok->next()->varId() == tok->tokAt(3)->varId()) {
+                selfInitializationError(tok, tok->strAt(1));
+            }
+        }
+    }
+}
+
+void CheckClass::selfInitializationError(const Token* tok, const std::string& name)
+{
+    reportError(tok, Severity::error, "selfInitialization", "Member variable '" + name + "' is initialized by itself.\n");
+}
+
+
+//---------------------------------------------------------------------------
+// Check for pure virtual function calls
+//---------------------------------------------------------------------------
+
 void CheckClass::checkPureVirtualFunctionCall()
 {
     const std::size_t functions = symbolDatabase->functionScopes.size();
@@ -2064,6 +2100,98 @@ void CheckClass::checkPureVirtualFunctionCall()
     }
 }
 
+const std::list<const Token *> & CheckClass::callsPureVirtualFunction(const Function & function,
+        std::map<const Function *, std::list<const Token *> > & callsPureVirtualFunctionMap)
+{
+    std::pair<std::map<const Function *, std::list<const Token *> >::iterator, bool > found =
+        callsPureVirtualFunctionMap.insert(std::pair<const Function *, std::list< const Token *> >(&function, std::list<const Token *>()));
+    std::list<const Token *> & pureFunctionCalls = found.first->second;
+    if (found.second) {
+        if (function.hasBody) {
+            for (const Token *tok = function.arg->link();
+                 tok && tok != function.functionScope->classEnd;
+                 tok = tok->next()) {
+                if (function.type != Function::eConstructor &&
+                    function.type != Function::eCopyConstructor &&
+                    function.type != Function::eMoveConstructor &&
+                    function.type != Function::eDestructor) {
+                    if ((Token::simpleMatch(tok, ") {") &&
+                         tok->link() &&
+                         Token::Match(tok->link()->previous(), "if|switch")) ||
+                        Token::simpleMatch(tok, "else {")
+                       ) {
+                        // Assume pure virtual function call is prevented by "if|else|switch" condition
+                        tok = tok->linkAt(1);
+                        continue;
+                    }
+                }
+                const Function * callFunction = tok->function();
+                if (!callFunction ||
+                    function.nestedIn != callFunction->nestedIn ||
+                    (tok->previous() && tok->previous()->str() == "."))
+                    continue;
+
+                if (tok->previous() &&
+                    tok->previous()->str() == "(") {
+                    const Token * prev = tok->previous();
+                    if (prev->previous() &&
+                        (_settings->library.ignorefunction(tok->str())
+                         || _settings->library.ignorefunction(prev->previous()->str())))
+                        continue;
+                }
+
+                if (isPureWithoutBody(*callFunction)) {
+                    pureFunctionCalls.push_back(tok);
+                    continue;
+                }
+
+                const std::list<const Token *> & pureFunctionCallsOfTok = callsPureVirtualFunction(*callFunction,
+                        callsPureVirtualFunctionMap);
+                if (!pureFunctionCallsOfTok.empty()) {
+                    pureFunctionCalls.push_back(tok);
+                    continue;
+                }
+            }
+        }
+    }
+    return pureFunctionCalls;
+}
+
+void CheckClass::getFirstPureVirtualFunctionCallStack(
+    std::map<const Function *, std::list<const Token *> > & callsPureVirtualFunctionMap,
+    const Token & pureCall,
+    std::list<const Token *> & pureFuncStack)
+{
+    if (isPureWithoutBody(*pureCall.function())) {
+        pureFuncStack.push_back(pureCall.function()->token);
+        return;
+    }
+    std::map<const Function *, std::list<const Token *> >::const_iterator found = callsPureVirtualFunctionMap.find(pureCall.function());
+    if (found == callsPureVirtualFunctionMap.end() ||
+        found->second.empty()) {
+        pureFuncStack.clear();
+        return;
+    }
+    const Token & firstPureCall = **found->second.begin();
+    pureFuncStack.push_back(&firstPureCall);
+    getFirstPureVirtualFunctionCallStack(callsPureVirtualFunctionMap, firstPureCall, pureFuncStack);
+}
+
+void CheckClass::callsPureVirtualFunctionError(
+    const Function & scopeFunction,
+    const std::list<const Token *> & tokStack,
+    const std::string &purefuncname)
+{
+    const char * scopeFunctionTypeName = getFunctionTypeName(scopeFunction.type);
+    reportError(tokStack, Severity::warning, "pureVirtualCall", "Call of pure virtual function '" + purefuncname + "' in " + scopeFunctionTypeName + ".\n"
+                "Call of pure virtual function '" + purefuncname + "' in " + scopeFunctionTypeName + ". The call will fail during runtime.");
+}
+
+
+//---------------------------------------------------------------------------
+// Check for members hiding inherited members with the same name
+//---------------------------------------------------------------------------
+
 void CheckClass::checkDuplInheritedMembers()
 {
     if (!_settings->isEnabled("warning"))
@@ -2112,91 +2240,3 @@ void CheckClass::duplInheritedMembersError(const Token *tok1, const Token* tok2,
                                 std::string(baseIsStruct ? "struct" : "class") + " '" + basename + "'.";
     reportError(toks, Severity::warning, "duplInheritedMember", message);
 }
-
-const std::list<const Token *> & CheckClass::callsPureVirtualFunction(const Function & function,
-        std::map<const Function *, std::list<const Token *> > & callsPureVirtualFunctionMap)
-{
-    std::pair<std::map<const Function *, std::list<const Token *> >::iterator , bool > found=
-        callsPureVirtualFunctionMap.insert(std::pair<const Function *, std::list< const Token *> >(&function,std::list<const Token *>()));
-    std::list<const Token *> & pureFunctionCalls=found.first->second;
-    if (found.second) {
-        if (function.hasBody) {
-            for (const Token *tok = function.arg->link();
-                 tok && tok != function.functionScope->classEnd;
-                 tok = tok->next()) {
-                if (function.type != Function::eConstructor &&
-                    function.type != Function::eCopyConstructor &&
-                    function.type != Function::eMoveConstructor &&
-                    function.type != Function::eDestructor) {
-                    if ((Token::simpleMatch(tok,") {") &&
-                         tok->link() &&
-                         Token::Match(tok->link()->previous(),"if|switch")) ||
-                        Token::simpleMatch(tok,"else {")
-                       ) {
-                        // Assume pure virtual function call is prevented by "if|else|switch" condition
-                        tok = tok->linkAt(1);
-                        continue;
-                    }
-                }
-                const Function * callFunction=tok->function();
-                if (!callFunction ||
-                    function.nestedIn != callFunction->nestedIn ||
-                    (tok->previous() && tok->previous()->str()=="."))
-                    continue;
-
-                if (tok->previous() &&
-                    tok->previous()->str()=="(") {
-                    const Token * prev = tok->previous();
-                    if (prev->previous() &&
-                        (_settings->library.ignorefunction(tok->str())
-                         || _settings->library.ignorefunction(prev->previous()->str())))
-                        continue;
-                }
-
-                if (isPureWithoutBody(*callFunction)) {
-                    pureFunctionCalls.push_back(tok);
-                    continue;
-                }
-
-                const std::list<const Token *> & pureFunctionCallsOfTok=callsPureVirtualFunction(*callFunction,
-                        callsPureVirtualFunctionMap);
-                if (!pureFunctionCallsOfTok.empty()) {
-                    pureFunctionCalls.push_back(tok);
-                    continue;
-                }
-            }
-        }
-    }
-    return pureFunctionCalls;
-}
-
-void CheckClass::getFirstPureVirtualFunctionCallStack(
-    std::map<const Function *, std::list<const Token *> > & callsPureVirtualFunctionMap,
-    const Token & pureCall,
-    std::list<const Token *> & pureFuncStack)
-{
-    if (isPureWithoutBody(*pureCall.function())) {
-        pureFuncStack.push_back(pureCall.function()->token);
-        return;
-    }
-    std::map<const Function *, std::list<const Token *> >::const_iterator found=callsPureVirtualFunctionMap.find(pureCall.function());
-    if (found==callsPureVirtualFunctionMap.end() ||
-        found->second.empty()) {
-        pureFuncStack.clear();
-        return;
-    }
-    const Token & firstPureCall=**found->second.begin();
-    pureFuncStack.push_back(&firstPureCall);
-    getFirstPureVirtualFunctionCallStack(callsPureVirtualFunctionMap, firstPureCall, pureFuncStack);
-}
-
-void CheckClass::callsPureVirtualFunctionError(
-    const Function & scopeFunction,
-    const std::list<const Token *> & tokStack,
-    const std::string &purefuncname)
-{
-    const char * scopeFunctionTypeName=getFunctionTypeName(scopeFunction.type);
-    reportError(tokStack, Severity::warning, "pureVirtualCall", "Call of pure virtual function '" + purefuncname + "' in " + scopeFunctionTypeName + ".\n"
-                "Call of pure virtual function '" + purefuncname + "' in " + scopeFunctionTypeName + ". The call will fail during runtime.");
-}
-

lib/checkclass.h

@@ -68,6 +68,7 @@ public:
         checkClass.operatorEqToSelf();
         checkClass.initializerListOrder();
         checkClass.initializationListUsage();
+        checkClass.checkSelfInitialization();
 
         checkClass.virtualDestructor();
         checkClass.checkConst();
@@ -116,8 +117,12 @@ public:
     /** @brief Check initializer list order */
     void initializerListOrder();
 
+    /** @brief Suggest using initialization list */
     void initializationListUsage();
 
+    /** @brief Check for initialization of a member with itself */
+    void checkSelfInitialization();
+
     void copyconstructors();
 
     /** @brief call of pure virtual funcion */
@@ -150,6 +155,7 @@ private:
     void checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname, bool suggestStatic);
     void initializerListError(const Token *tok1,const Token *tok2, const std::string & classname, const std::string &varname);
     void suggestInitializationList(const Token *tok, const std::string& varname);
+    void selfInitializationError(const Token* tok, const std::string& name);
     void callsPureVirtualFunctionError(const Function & scopeFunction, const std::list<const Token *> & tokStack, const std::string &purefuncname);
     void duplInheritedMembersError(const Token* tok1, const Token* tok2, const std::string &derivedname, const std::string &basename, const std::string &variablename, bool derivedIsStruct, bool baseIsStruct);
 
@@ -174,6 +180,7 @@ private:
         c.checkConstError(0, "class", "function", true);
         c.initializerListError(0, 0, "class", "variable");
         c.suggestInitializationList(0, "variable");
+        c.selfInitializationError(0, "var");
         c.duplInheritedMembersError(0, 0, "class", "class", "variable", false, false);
     }
 
@@ -196,6 +203,7 @@ private:
                "* Constness for member functions\n"
                "* Order of initializations\n"
                "* Suggest usage of initialization list\n"
+               "* Initialization of a member with itself\n"
                "* Suspicious subtraction from 'this'\n"
                "* Call of pure virtual function in constructor/destructor\n"
                "* Duplicated inherited data members\n";

test/testclass.cpp

@@ -57,6 +57,7 @@ private:
         TEST_CASE(noConstructor7); // ticket #4391
         TEST_CASE(noConstructor8); // ticket #4404
         TEST_CASE(noConstructor9); // ticket #4419
+        TEST_CASE(forwardDeclaration); // ticket #4290/#3190
 
         TEST_CASE(operatorEq1);
         TEST_CASE(operatorEq2);
@@ -178,8 +179,7 @@ private:
 
         TEST_CASE(initializerListOrder);
         TEST_CASE(initializerListUsage);
-
+        TEST_CASE(selfInitialization);
-        TEST_CASE(forwardDeclaration); // ticket #4290/#3190
 
         TEST_CASE(pureVirtualFunctionCall);
         TEST_CASE(pureVirtualFunctionCallOtherClass);
@@ -2019,12 +2019,12 @@ private:
         ASSERT_EQUALS("[test.cpp:3]: (error) Class 'Base' which is inherited by class 'Derived' does not have a virtual destructor.\n", errout.str());
     }
 
-    void checkNoConstructor(const char code[], const char* level="style") {
+    void checkNoConstructor(const char code[]) {
         // Clear the error log
         errout.str("");
 
         Settings settings;
-        settings.addEnabled(level);
+        settings.addEnabled("style");
 
         // Tokenize..
         Tokenizer tokenizer(&settings, this);
@@ -2124,6 +2124,22 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    // ticket #4290 "False Positive: style (noConstructor): The class 'foo' does not have a constructor."
+    // ticket #3190 "SymbolDatabase: Parse of sub class constructor fails"
+    void forwardDeclaration() {
+        checkNoConstructor("class foo;\n"
+                           "int bar;\n");
+        ASSERT_EQUALS("", errout.str());
+
+        checkNoConstructor("class foo;\n"
+                           "class foo;\n");
+        ASSERT_EQUALS("", errout.str());
+
+        checkNoConstructor("class foo{};\n"
+                           "class foo;\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
     void checkNoMemset(const char code[], bool load_std_cfg = false) {
         // Clear the error log
         errout.str("");
@@ -5826,19 +5842,52 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-    // ticket #4290 "False Positive: style (noConstructor): The class 'foo' does not have a constructor."
-    // ticket #3190 "SymbolDatabase: Parse of sub class constructor fails"
-    void forwardDeclaration() {
-        checkConst("class foo;\n"
-                   "int bar;\n");
-        ASSERT_EQUALS("", errout.str());
 
-        checkConst("class foo;\n"
+    void checkSelfInitialization(const char code []) {
-                   "class foo;\n");
+        // Clear the error log
-        ASSERT_EQUALS("", errout.str());
+        errout.str("");
 
-        checkConst("class foo{};\n"
+        // Check..
-                   "class foo;\n");
+        Settings settings;
+
+        // Tokenize..
+        Tokenizer tokenizer(&settings, this);
+        std::istringstream istr(code);
+        tokenizer.tokenize(istr, "test.cpp");
+        tokenizer.simplifyTokenList2();
+
+        CheckClass checkClass(&tokenizer, &settings, this);
+        checkClass.checkSelfInitialization();
+    }
+
+    void selfInitialization() {
+        checkSelfInitialization("class Fred {\n"
+                                "    int i;\n"
+                                "    Fred() : i(i) {\n"
+                                "    }\n"
+                                "};");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Member variable 'i' is initialized by itself.\n", errout.str());
+
+        checkSelfInitialization("class Fred {\n"
+                                "    int i;\n"
+                                "    Fred();\n"
+                                "};\n"
+                                "Fred::Fred() : i(i) {\n"
+                                "}");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Member variable 'i' is initialized by itself.\n", errout.str());
+
+        checkSelfInitialization("class Fred {\n"
+                                "    std::string s;\n"
+                                "    Fred() : s(s) {\n"
+                                "    }\n"
+                                "};");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Member variable 's' is initialized by itself.\n", errout.str());
+
+        checkSelfInitialization("class Fred {\n"
+                                "    std::string s;\n"
+                                "    Fred(const std::string& s) : s(s) {\n"
+                                "    }\n"
+                                "};");
         ASSERT_EQUALS("", errout.str());
     }
 
@@ -6052,7 +6101,7 @@ private:
         ASSERT_THROW(checkNoConstructor("struct R1 {\n"
                                         "  int a;\n"
                                         "  R1 () : a { }\n"
-                                        "};\n", "warning"), InternalError);
+                                        "};\n"), InternalError);
     }
 };