New check: Suggest to use initialization list instead of assignment in constructor. (#489)

lib/checkclass.cpp

@@ -495,6 +495,54 @@ void CheckClass::operatorEqVarError(const Token *tok, const std::string &classna
     reportError(tok, Severity::warning, "operatorEqVarError", "Member variable '" + classname + "::" + varname + "' is not assigned a value in '" + classname + "::operator=" + "'");
 }
 
+//---------------------------------------------------------------------------
+// ClassCheck: Use initialization list instead of assignment
+//---------------------------------------------------------------------------
+
+void CheckClass::initializationListUsage()
+{
+    if (!_settings->isEnabled("performance"))
+        return;
+
+    for (std::list<Scope>::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
+        // Check every constructor
+        if (scope->type != Scope::eFunction || !scope->function || (scope->function->type != Function::eConstructor && scope->function->type != Function::eCopyConstructor))
+            continue;
+
+        Scope* owner = scope->functionOf;
+        for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) {
+            if (Token::Match(tok, "%var% (")) // Assignments might depend on this function call or if/for/while/switch statment from now on.
+                break;
+            if (tok->varId() && Token::Match(tok, "%var% = %any%")) {
+                const Variable* var = symbolDatabase->getVariableFromVarId(tok->varId());
+                if (var && var->scope() == owner) {
+                    bool allowed = true;
+                    for (const Token* tok2 = tok->tokAt(2); tok2->str() != ";"; tok2 = tok2->next()) {
+                        if (tok2->varId()) {
+                            const Variable* var2 = symbolDatabase->getVariableFromVarId(tok2->varId());
+                            if (var2 && var2->scope() == owner) { // Is there a dependency between two member variables?
+                                allowed = false;
+                                break;
+                            }
+                        }
+                    }
+                    if (!allowed)
+                        continue;
+                    suggestInitializationList(tok, tok->str());
+                }
+            }
+        }
+    }
+}
+
+void CheckClass::suggestInitializationList(const Token* tok, const std::string& name)
+{
+    reportError(tok, Severity::performance, "useInitializationList", "Variable '" + name + "' is assigned in constructor body. Consider to perform initalization in initialization list.\n"
+                "When an object of a class is created, the constructors of all member variables are called consecutivly "
+                "in the order the variables are declared, even if you don't explicitly write them to the initialization list. You "
+                "could avoid assigning '" + name + "' a value by passing the value to the constructor in the initialization list.");
+}
+
 //---------------------------------------------------------------------------
 // ClassCheck: Unused private functions
 //---------------------------------------------------------------------------
@@ -1524,7 +1572,7 @@ struct VarInfo {
     const Token *tok;
 };
 
-void CheckClass::initializerList()
+void CheckClass::initializerListOrder()
 {
     if (!_settings->isEnabled("style"))
         return;

lib/checkclass.h

@@ -60,7 +60,8 @@ public:
         checkClass.operatorEqRetRefThis();
         checkClass.thisSubtraction();
         checkClass.operatorEqToSelf();
-        checkClass.initializerList();
+        checkClass.initializerListOrder();
+        checkClass.initializationListUsage();
 
         checkClass.virtualDestructor();
         checkClass.checkConst();
@@ -103,7 +104,9 @@ public:
     void checkConst();
 
     /** @brief Check initializer list order */
-    void initializerList();
+    void initializerListOrder();
+
+    void initializationListUsage();
 
 private:
     const SymbolDatabase *symbolDatabase;
@@ -122,6 +125,7 @@ private:
     void checkConstError(const Token *tok, const std::string &classname, const std::string &funcname);
     void checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname);
     void initializerListError(const Token *tok1,const Token *tok2, const std::string & classname, const std::string &varname);
+    void suggestInitializationList(const Token *tok, const std::string& name);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckClass c(0, settings, errorLogger);
@@ -137,6 +141,7 @@ private:
         c.operatorEqToSelfError(0);
         c.checkConstError(0, "class", "function");
         c.initializerListError(0, 0, "class", "variable");
+        c.suggestInitializationList(0, "variable");
     }
 
     std::string myName() const {

test/testclass.cpp

@@ -157,7 +157,8 @@ private:
         TEST_CASE(constFriend); // ticket #1921 - fp for friend function
         TEST_CASE(constUnion);  // ticket #2111 - fp when there is a union
 
-        TEST_CASE(initializerList);
+        TEST_CASE(initializerListOrder);
+        TEST_CASE(initializerListUsage);
     }
 
     // Check the operator Equal
@@ -5075,7 +5076,7 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-    void checkInitializerList(const char code[]) {
+    void checkInitializerListOrder(const char code[]) {
         // Clear the error log
         errout.str("");
 
@@ -5091,18 +5092,74 @@ private:
         tokenizer.simplifyTokenList();
 
         CheckClass checkClass(&tokenizer, &settings, this);
-        checkClass.initializerList();
+        checkClass.initializerListOrder();
     }
 
-    void initializerList() {
+    void initializerListOrder() {
-        checkInitializerList("class Fred {\n"
+        checkInitializerListOrder("class Fred {\n"
-                             "    int a, b, c;\n"
+                                  "    int a, b, c;\n"
-                             "public:\n"
+                                  "public:\n"
-                             "    Fred() : c(0), b(0), a(0) { }\n"
+                                  "    Fred() : c(0), b(0), a(0) { }\n"
-                             "};");
+                                  "};");
         ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2]: (style, inconclusive) Member variable 'Fred::b' is in the wrong order in the initializer list.\n"
                       "[test.cpp:4] -> [test.cpp:2]: (style, inconclusive) Member variable 'Fred::a' is in the wrong order in the initializer list.\n", errout.str());
     }
+
+    void checkInitializationListUsage(const char code[]) {
+        // Clear the error log
+        errout.str("");
+
+        // Check..
+        Settings settings;
+        settings.addEnabled("performance");
+
+        // Tokenize..
+        Tokenizer tokenizer(&settings, this);
+        std::istringstream istr(code);
+        tokenizer.tokenize(istr, "test.cpp");
+        tokenizer.simplifyTokenList();
+
+        CheckClass checkClass(&tokenizer, &settings, this);
+        checkClass.initializationListUsage();
+    }
+
+    void initializerListUsage() {
+        checkInitializationListUsage("class Fred {\n"
+                                     "    int a;\n"
+                                     "    Fred() { a = 0; }\n"
+                                     "};");
+        ASSERT_EQUALS("[test.cpp:3]: (performance) Variable 'a' is assigned in constructor body. Consider to perform initalization in initialization list.\n", errout.str());
+
+        checkInitializationListUsage("class Fred {\n"
+                                     "    std::string s;\n"
+                                     "    Fred() { a = 0; s = \"foo\"; }\n"
+                                     "};");
+        ASSERT_EQUALS("[test.cpp:3]: (performance) Variable 's' is assigned in constructor body. Consider to perform initalization in initialization list.\n", errout.str());
+
+        checkInitializationListUsage("class Fred {\n"
+                                     "    int a;\n"
+                                     "    Fred() { initB(); a = b; }\n"
+                                     "};");
+        ASSERT_EQUALS("", errout.str());
+
+        checkInitializationListUsage("class Fred {\n"
+                                     "    int a;\n"
+                                     "    Fred() : a(0) { if(b) a = 0; }\n"
+                                     "};");
+        ASSERT_EQUALS("", errout.str());
+
+        checkInitializationListUsage("class Fred {\n"
+                                     "    int a[5];\n"
+                                     "    Fred() { for(int i = 0; i < 5; i++) a[i] = 0; }\n"
+                                     "};");
+        ASSERT_EQUALS("", errout.str());
+
+        checkInitializationListUsage("class Fred {\n"
+                                     "    int a; int b;\n"
+                                     "    Fred() : b(5) { a = b; }\n" // Don't issue a message here: You actually could move it to the initalization list, but it would cause problems if you change the order of the variable declarations.
+                                     "};");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestClass)