diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index a3959f523..0cd056e0b 100644 --- a/lib/checkclass.cpp +++ b/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::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; diff --git a/lib/checkclass.h b/lib/checkclass.h index b06e9307d..24c60a379 100644 --- a/lib/checkclass.h +++ b/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 { diff --git a/test/testclass.cpp b/test/testclass.cpp index 8d0425f6d..8f61295e0 100644 --- a/test/testclass.cpp +++ b/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() { - checkInitializerList("class Fred {\n" - " int a, b, c;\n" - "public:\n" - " Fred() : c(0), b(0), a(0) { }\n" - "};"); + void initializerListOrder() { + checkInitializerListOrder("class Fred {\n" + " int a, b, c;\n" + "public:\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)