From 1bc8a2b6ba14068f545cdcee9f63ec1df1d1e9c7 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Thu, 2 Dec 2010 07:35:01 +0100 Subject: [PATCH] Fixed #2172 (False positive: struct is not initialized in constructor) --- lib/checkclass.cpp | 17 +++- lib/symboldatabase.cpp | 201 ++++++++++++++++++++++++++++++++++++-- lib/symboldatabase.h | 20 ++-- test/testclass.cpp | 101 ++++++++++++++++++- test/testconstructors.cpp | 33 ++++--- 5 files changed, 339 insertions(+), 33 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index ada77cf26..a16c27225 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -122,16 +122,25 @@ void CheckClass::constructors() std::list::const_iterator var; for (var = info->varlist.begin(); var != info->varlist.end(); ++var) { - // skip classes for regular constructor - if (var->isClass && func->type == SymbolDatabase::Func::Constructor) - continue; - if (var->assign || var->init || var->isStatic) continue; if (var->isConst && var->token->previous()->str() != "*") continue; + // Check if this is a class constructor + if (var->isClass && func->type == SymbolDatabase::Func::Constructor) + { + // Unknown type so assume it is initialized + if (!var->type) + continue; + + // Known type that doesn't need initialization or + // known type that has member variables of an unknown type + else if (var->type->needInitialization != SymbolDatabase::SpaceInfo::True) + continue; + } + // It's non-static and it's not initialized => error if (func->type == SymbolDatabase::Func::OperatorEqual) { diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 5738861fb..52f601c4e 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -310,12 +310,13 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti std::list::iterator it; + // fill in base class info for (it = spaceInfoList.begin(); it != spaceInfoList.end(); ++it) { info = *it; // skip namespaces and functions - if (info->type == SpaceInfo::Namespace || info->type == SpaceInfo::Function) + if (!info->isClassOrStruct()) continue; // finish filling in base class info @@ -346,9 +347,127 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti } } } + } - // find variables - info->getVarList(); + // fill in variable info + for (it = spaceInfoList.begin(); it != spaceInfoList.end(); ++it) + { + info = *it; + + // skip functions + if (info->type != SpaceInfo::Function) + { + // find variables + info->getVarList(); + } + } + + // determine if user defined type needs initialization + unsigned int unknowns = 0; // stop checking when there are no unknowns + unsigned int retry = 0; // bail if we don't resolve all the variable types for some reason + + do + { + unknowns = 0; + + for (it = spaceInfoList.begin(); it != spaceInfoList.end(); ++it) + { + info = *it; + + if (info->isClassOrStruct() && info->needInitialization == SpaceInfo::Unknown) + { + // check for default constructor + bool hasDefaultConstructor = false; + + std::list::const_iterator func; + + for (func = info->functionList.begin(); func != info->functionList.end(); ++func) + { + if (func->type == SymbolDatabase::Func::Constructor) + { + // check for no arguments: func ( ) + /** @todo check for arguents with default values someday */ + if (func->argDef->next() == func->argDef->link()) + { + hasDefaultConstructor = true; + break; + } + } + } + + // User defined types with user defined defaut constructor doesn't need initialization. + // We assume the default constructor initializes everything. + // Another check will figure out if the constructor actually initializes everything. + if (hasDefaultConstructor) + info->needInitialization = SpaceInfo::False; + + // check each member variable to see if it needs initialization + else + { + bool needInitialization = false; + bool unknown = false; + + std::list::const_iterator var; + for (var = info->varlist.begin(); var != info->varlist.end(); ++var) + { + if (var->isClass) + { + if (var->type) + { + // does this type need initialization? + if (var->type->needInitialization == SpaceInfo::True) + needInitialization = true; + else if (var->type->needInitialization == SpaceInfo::Unknown) + unknown = true; + } + } + else + needInitialization = true; + } + + if (!unknown) + { + if (needInitialization) + info->needInitialization = SpaceInfo::True; + else + info->needInitialization = SpaceInfo::False; + } + + if (info->needInitialization == SpaceInfo::Unknown) + unknowns++; + } + } + } + + retry++; + } + while (unknowns && retry < 100); + + // this shouldn't happen so output a debug warning + if (retry == 100 && _settings->debugwarnings) + { + for (it = spaceInfoList.begin(); it != spaceInfoList.end(); ++it) + { + info = *it; + + if (info->isClassOrStruct() && info->needInitialization == SpaceInfo::Unknown) + { + std::list locationList; + ErrorLogger::ErrorMessage::FileLocation loc; + loc.line = info->classDef->linenr(); + loc.setfile(_tokenizer->file(info->classDef)); + locationList.push_back(loc); + + const ErrorLogger::ErrorMessage errmsg(locationList, + Severity::debug, + "SymbolDatabase::SymbolDatabase couldn't resolve all user defined types.", + "debug"); + if (_errorLogger) + _errorLogger->reportErr(errmsg); + else + Check::reportError(errmsg); + } + } } } @@ -745,7 +864,8 @@ SymbolDatabase::SpaceInfo::SpaceInfo(SymbolDatabase *check_, const Token *classD classStart(NULL), classEnd(NULL), nestedIn(nestedIn_), - numConstructors(0) + numConstructors(0), + needInitialization(SpaceInfo::Unknown) { if (!classDef) { @@ -874,9 +994,12 @@ void SymbolDatabase::SpaceInfo::getVarList() // Search for start of statement.. else if (!tok->previous() || !Token::Match(tok->previous(), ";|{|}|public:|protected:|private:")) continue; + else if (Token::Match(tok, ";|{|}")) + continue; // This is the start of a statement - const Token *vartok = 0; + const Token *vartok = NULL; + const Token *typetok = NULL; // Is it const..? bool isConst = false; @@ -913,7 +1036,10 @@ void SymbolDatabase::SpaceInfo::getVarList() if (Token::Match(tok, "%type% %var% ;|:")) { if (!tok->isStandardType()) + { isClass = true; + typetok = tok; + } vartok = tok->next(); tok = vartok->next(); @@ -922,31 +1048,37 @@ void SymbolDatabase::SpaceInfo::getVarList() { isClass = true; vartok = tok->tokAt(3); + typetok = vartok->previous(); tok = vartok->next(); } else if (Token::Match(tok, ":: %type% :: %type% %var% ;")) { isClass = true; vartok = tok->tokAt(4); + typetok = vartok->previous(); tok = vartok->next(); } else if (Token::Match(tok, "%type% :: %type% :: %type% %var% ;")) { isClass = true; vartok = tok->tokAt(5); + typetok = vartok->previous(); tok = vartok->next(); } else if (Token::Match(tok, ":: %type% :: %type% :: %type% %var% ;")) { isClass = true; vartok = tok->tokAt(6); + typetok = vartok->previous(); tok = vartok->next(); } // Structure? else if (Token::Match(tok, "struct|union %type% %var% ;")) { + isClass = true; vartok = tok->tokAt(2); + typetok = vartok->previous(); tok = vartok->next(); } @@ -976,7 +1108,10 @@ void SymbolDatabase::SpaceInfo::getVarList() else if (Token::Match(tok, "%type% %var% [") && tok->next()->str() != "operator") { if (!tok->isStandardType()) + { isClass = true; + typetok = tok; + } vartok = tok->next(); tok = vartok->next()->link()->next(); @@ -1013,10 +1148,15 @@ void SymbolDatabase::SpaceInfo::getVarList() // find matching ">" int level = 0; + const Token *tok1 = NULL; for (; tok; tok = tok->next()) { if (tok->str() == "<") + { + if (level == 0) + tok1 = tok->previous(); level++; + } else if (tok->str() == ">") { level--; @@ -1040,12 +1180,14 @@ void SymbolDatabase::SpaceInfo::getVarList() { isClass = true; vartok = tok->next(); + typetok = tok1; tok = vartok->next(); } else if (tok && (Token::Match(tok, "> :: %type% %var% ;") || Token::Match(tok, ">> :: %type% %var% ;"))) { isClass = true; vartok = tok->tokAt(3); + typetok = vartok->previous(); tok = vartok->next(); } else if (tok && (Token::Match(tok, "> * %var% ;") || Token::Match(tok, ">> * %var% ;"))) @@ -1076,13 +1218,60 @@ void SymbolDatabase::SpaceInfo::getVarList() Check::reportError(errmsg); } - addVar(vartok, varaccess, isMutable, isStatic, isConst, isClass); + const SpaceInfo *spaceInfo = NULL; + + if (isClass) + spaceInfo = check->findVarType(this, typetok); + + addVar(vartok, varaccess, isMutable, isStatic, isConst, isClass, spaceInfo); } } } //--------------------------------------------------------------------------- +const SymbolDatabase::SpaceInfo *SymbolDatabase::findVarType(const SpaceInfo *start, const Token *type) const +{ + std::list::const_iterator it; + + for (it = spaceInfoList.begin(); it != spaceInfoList.end(); ++it) + { + const SpaceInfo *info = *it; + + // skip namespaces and functions + if (info->type == SpaceInfo::Namespace || info->type == SpaceInfo::Function || info->type == SpaceInfo::Global) + continue; + + // do the names match? + if (info->className == type->str()) + { + // check if type does not have a namespace + if (type->previous()->str() != "::") + { + const SpaceInfo *parent = start; + + // check if in same namespace + while (parent && parent != info->nestedIn) + parent = parent->nestedIn; + + if (info->nestedIn == parent) + return info; + } + + // type has a namespace + else + { + // FIXME check if namespace path matches supplied path + return info; + } + } + } + + return NULL; +} + +//--------------------------------------------------------------------------- + SymbolDatabase::SpaceInfo * SymbolDatabase::SpaceInfo::findInNestedList(const std::string & name) { std::list::iterator it; diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 4cecb6105..f12f46147 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -41,11 +41,13 @@ public: SymbolDatabase(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger); ~SymbolDatabase(); + class SpaceInfo; + /** @brief Information about a member variable. Used when checking for uninitialized variables */ class Var { public: - Var(const Token *token_, unsigned int index_, AccessControl access_, bool mutable_, bool static_, bool const_, bool class_) + Var(const Token *token_, unsigned int index_, AccessControl access_, bool mutable_, bool static_, bool const_, bool class_, const SpaceInfo *type_) : token(token_), index(index_), assign(false), @@ -54,7 +56,8 @@ public: isMutable(mutable_), isStatic(static_), isConst(const_), - isClass(class_) + isClass(class_), + type(type_) { } @@ -84,6 +87,9 @@ public: /** @brief is this variable a class (or unknown type)? */ bool isClass; + + /** @brief pointer to user defined type info (for known types) */ + const SpaceInfo *type; }; class Func @@ -129,8 +135,6 @@ public: Type type; // constructor, destructor, ... }; - class SpaceInfo; - struct BaseInfo { AccessControl access; // public/protected/private @@ -148,6 +152,7 @@ public: { public: enum SpaceType { Global, Class, Struct, Union, Namespace, Function }; + enum NeedInitialization { Unknown, True, False }; SpaceInfo(SymbolDatabase *check_, const Token *classDef_, SpaceInfo *nestedIn_); @@ -165,6 +170,7 @@ public: std::list nestedList; AccessControl access; unsigned int numConstructors; + NeedInitialization needInitialization; bool isClassOrStruct() const { @@ -189,9 +195,9 @@ public: */ void initVar(const std::string &varname); - void addVar(const Token *token_, AccessControl access_, bool mutable_, bool static_, bool const_, bool class_) + void addVar(const Token *token_, AccessControl access_, bool mutable_, bool static_, bool const_, bool class_, const SpaceInfo *type_) { - varlist.push_back(Var(token_, varlist.size(), access_, mutable_, static_, const_, class_)); + varlist.push_back(Var(token_, varlist.size(), access_, mutable_, static_, const_, class_, type_)); } /** @@ -246,6 +252,8 @@ private: bool isFunction(const Token *tok, const Token **funcStart, const Token **argStart) const; bool argsMatch(const Token *first, const Token *second, const std::string &path, unsigned int depth) const; + const SpaceInfo *findVarType(const SpaceInfo *start, const Token *type) const; + const Tokenizer *_tokenizer; const Settings *_settings; ErrorLogger *_errorLogger; diff --git a/test/testclass.cpp b/test/testclass.cpp index 35e97109c..e1e52f94e 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -69,6 +69,8 @@ private: TEST_CASE(uninitVarArray5); TEST_CASE(uninitVarArray6); TEST_CASE(uninitVarArray2D); + TEST_CASE(uninitVarStruct1); // ticket #2172 + TEST_CASE(uninitVarStruct2); // ticket #838 TEST_CASE(uninitMissingFuncDef); // can't expand function in constructor TEST_CASE(privateCtor1); // If constructor is private.. TEST_CASE(privateCtor2); // If constructor is private.. @@ -1594,8 +1596,7 @@ private: " };\n" " Bar bars[2];\n" "};\n"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (warning) Member variable not initialized in the constructor 'Foo::bars'\n", errout.str()); - ASSERT_EQUALS("", errout.str()); // So we notice if something is reported. + ASSERT_EQUALS("[test.cpp:4]: (warning) Member variable not initialized in the constructor 'Foo::bars'\n", errout.str()); } void uninitVar4() @@ -2083,6 +2084,102 @@ private: ASSERT_EQUALS("", errout.str()); } + void uninitVarStruct1() // ticket #2172 + { + checkUninitVar("class A\n" + "{\n" + "private:\n" + " struct B {\n" + " std::string str1;\n" + " std::string str2;\n" + " }\n" + " struct B b;\n" + "public:\n" + " A() {\n" + " }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkUninitVar("class A\n" + "{\n" + "private:\n" + " struct B {\n" + " char *str1;\n" + " char *str2;\n" + " }\n" + " struct B b;\n" + "public:\n" + " A() {\n" + " }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:10]: (warning) Member variable not initialized in the constructor 'A::b'\n", errout.str()); + + checkUninitVar("class A\n" + "{\n" + "private:\n" + " struct B {\n" + " char *str1;\n" + " char *str2;\n" + " B() : str1(NULL), str2(NULL) { }\n" + " }\n" + " struct B b;\n" + "public:\n" + " A() {\n" + " }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + } + + void uninitVarStruct2() // ticket #838 + { + checkUninitVar("struct POINT\n" + "{\n" + " int x;\n" + " int y;\n" + "};\n" + "class Fred\n" + "{\n" + "private:\n" + " POINT p;\n" + "public:\n" + " Fred()\n" + " { }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:11]: (warning) Member variable not initialized in the constructor 'Fred::p'\n", errout.str()); + + checkUninitVar("struct POINT\n" + "{\n" + " int x;\n" + " int y;\n" + " POINT();\n" + "};\n" + "class Fred\n" + "{\n" + "private:\n" + " POINT p;\n" + "public:\n" + " Fred()\n" + " { }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkUninitVar("struct POINT\n" + "{\n" + " int x;\n" + " int y;\n" + " POINT() :x(0), y(0) { }\n" + "};\n" + "class Fred\n" + "{\n" + "private:\n" + " POINT p;\n" + "public:\n" + " Fred()\n" + " { }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + } + void uninitMissingFuncDef() { // Unknown member function diff --git a/test/testconstructors.cpp b/test/testconstructors.cpp index b7e72eb61..325eda2a9 100644 --- a/test/testconstructors.cpp +++ b/test/testconstructors.cpp @@ -820,12 +820,12 @@ private: "public:\n" " A();\n" " struct B {\n" - " B();\n" + " B(int x);\n" " struct C {\n" - " C();\n" + " C(int y);\n" " struct D {\n" " int d;\n" - " D();\n" + " D(int z);\n" " };\n" " int c;\n" " };\n" @@ -836,10 +836,11 @@ private: " B b;\n" "};\n" "A::A(){}\n" - "A::B::B(){}\n" - "A::B::C::C(){}\n" - "A::B::C::D::D(){}\n"); + "A::B::B(int x){}\n" + "A::B::C::C(int y){}\n" + "A::B::C::D::D(int z){}\n"); ASSERT_EQUALS("[test.cpp:20]: (warning) Member variable not initialized in the constructor 'A::a'\n" + "[test.cpp:20]: (warning) Member variable not initialized in the constructor 'A::b'\n" "[test.cpp:21]: (warning) Member variable not initialized in the constructor 'B::b'\n" "[test.cpp:22]: (warning) Member variable not initialized in the constructor 'C::c'\n" "[test.cpp:23]: (warning) Member variable not initialized in the constructor 'D::d'\n", errout.str()); @@ -848,9 +849,9 @@ private: "public:\n" " A();\n" " struct B {\n" - " B();\n" + " B(int x);\n" " struct C {\n" - " C();\n" + " C(int y);\n" " struct D {\n" " D(const D &);\n" " int d;\n" @@ -864,10 +865,11 @@ private: " B b;\n" "};\n" "A::A(){}\n" - "A::B::B(){}\n" - "A::B::C::C(){}\n" + "A::B::B(int x){}\n" + "A::B::C::C(int y){}\n" "A::B::C::D::D(const A::B::C::D & d){}\n"); ASSERT_EQUALS("[test.cpp:20]: (warning) Member variable not initialized in the constructor 'A::a'\n" + "[test.cpp:20]: (warning) Member variable not initialized in the constructor 'A::b'\n" "[test.cpp:21]: (warning) Member variable not initialized in the constructor 'B::b'\n" "[test.cpp:22]: (warning) Member variable not initialized in the constructor 'C::c'\n" "[test.cpp:23]: (warning) Member variable not initialized in the constructor 'D::d'\n", errout.str()); @@ -876,11 +878,11 @@ private: "public:\n" " A();\n" " struct B {\n" - " B();\n" + " B(int x);\n" " struct C {\n" - " C();\n" + " C(int y);\n" " struct D {\n" - " struct E { };\n" + " struct E { int e; };\n" " struct E d;\n" " D(const E &);\n" " };\n" @@ -893,10 +895,11 @@ private: " B b;\n" "};\n" "A::A(){}\n" - "A::B::B(){}\n" - "A::B::C::C(){}\n" + "A::B::B(int x){}\n" + "A::B::C::C(int y){}\n" "A::B::C::D::D(const A::B::C::D::E & e){}\n"); ASSERT_EQUALS("[test.cpp:21]: (warning) Member variable not initialized in the constructor 'A::a'\n" + "[test.cpp:21]: (warning) Member variable not initialized in the constructor 'A::b'\n" "[test.cpp:22]: (warning) Member variable not initialized in the constructor 'B::b'\n" "[test.cpp:23]: (warning) Member variable not initialized in the constructor 'C::c'\n" "[test.cpp:24]: (warning) Member variable not initialized in the constructor 'D::d'\n", errout.str());