From e2bab4b6a3cb3b1502c59b96047ef5c2db864ab1 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Thu, 24 May 2012 08:40:43 -0700 Subject: [PATCH] Implemented Function::nestedIn to be able to identify the scope the function belongs to, even if Function::functionScope.functionOf is not available. Refactorized usage of SymbolDatabase in checkOther: - Don't copy Function instances in checkExpressionRange - Simplifications by more accurate usage of information in database --- lib/checkother.cpp | 105 +++++++++++++++--------------------- lib/checkother.h | 4 +- lib/symboldatabase.cpp | 18 +++++-- lib/symboldatabase.h | 2 + test/testother.cpp | 54 ++++++++----------- test/testsymboldatabase.cpp | 6 +-- 6 files changed, 85 insertions(+), 104 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 65b7b464d..1a8dc7646 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1665,27 +1665,23 @@ void CheckOther::checkConstantFunctionParameter() const SymbolDatabase * const symbolDatabase = _tokenizer->getSymbolDatabase(); - for (std::list::const_iterator i = symbolDatabase->scopeList.begin(); i != symbolDatabase->scopeList.end(); ++i) { - if (i->type == Scope::eFunction && i->function && i->function->arg) { - for (const Token* tok = i->function->arg->next(); tok; tok = tok->nextArgument()) { - // TODO: False negatives. This pattern only checks for string. - // Investigate if there are other classes in the std - // namespace and add them to the pattern. There are - // streams for example (however it seems strange with - // const stream parameter). - if (Token::Match(tok, "const std :: string %var% [,)]")) { - passedByValueError(tok, tok->strAt(4)); - } else if (Token::Match(tok, "const std :: %type% < std| ::| %type% > %var% [,)]")) { - passedByValueError(tok, Token::findsimplematch(tok->tokAt(5), ">")->strAt(1)); - } else if (Token::Match(tok, "const std :: %type% < std| ::| %type% , std| ::| %type% > %var% [,)]")) { - passedByValueError(tok, Token::findsimplematch(tok->tokAt(7), ">")->strAt(1)); - } else if (Token::Match(tok, "const %type% %var% [,)]")) { - // Check if type is a struct or class. - if (symbolDatabase->isClassOrStruct(tok->strAt(1))) { - passedByValueError(tok, tok->strAt(2)); - } - } - } + for (unsigned int i = 1; i < symbolDatabase->getVariableListSize(); i++) { + const Variable* var = symbolDatabase->getVariableFromVarId(i); + if (!var || !var->isArgument() || !var->isClass() || !var->isConst() || var->isPointer() || var->isArray() || var->isReference()) + continue; + + const Token* tok = var->typeStartToken()->next(); + // TODO: False negatives. This pattern only checks for string. + // Investigate if there are other classes in the std + // namespace and add them to the pattern. There are + // streams for example (however it seems strange with + // const stream parameter). + if (Token::Match(tok, "std :: string|wstring")) { + passedByValueError(tok, var->name()); + } else if (Token::Match(tok, "std :: %type% <")) { + passedByValueError(tok, var->name()); + } else if (var->type() || symbolDatabase->isClassOrStruct(tok->str())) { // Check if type is a struct or class. + passedByValueError(tok, var->name()); } } } @@ -1851,7 +1847,7 @@ void CheckOther::strPlusChar() for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { if (Token::Match(tok, "[=(] %str% + %any%")) { // char constant.. - if (tok->strAt(3)[0] == '\'') + if (tok->tokAt(3)->type() == Token::eChar) strPlusCharError(tok->next()); // char variable.. @@ -1960,6 +1956,7 @@ void CheckOther::mathfunctionCallError(const Token *tok, const unsigned int numP } else reportError(tok, Severity::error, "wrongmathcall", "Passing value " " to " "() leads to undefined result"); } + //--------------------------------------------------------------------------- //--------------------------------------------------------------------------- void CheckOther::checkCCTypeFunctions() @@ -1976,6 +1973,7 @@ void CheckOther::cctypefunctionCallError(const Token *tok, const std::string &fu { reportError(tok, Severity::error, "wrongcctypecall", "Passing value " + value + " to " + functionName + "() cause undefined behavior, which may lead to a crash"); } + //--------------------------------------------------------------------------- //--------------------------------------------------------------------------- /** Is there a function with given name? */ @@ -2178,29 +2176,20 @@ void CheckOther::checkDuplicateBranch() std::list::const_iterator scope; for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { - const Token* tok = scope->classDef; - - if ((scope->type != Scope::eIf && scope->type != Scope::eElseIf) || !tok) + if (scope->type != Scope::eIf && scope->type != Scope::eElseIf) continue; - if (tok->str() == "else") - tok = tok->next(); - // check all the code in the function for if (..) else - if (tok && tok->next() && Token::simpleMatch(tok->next()->link(), ") {") && - Token::simpleMatch(tok->next()->link()->next()->link(), "} else {")) { + if (Token::simpleMatch(scope->classEnd, "} else {")) { // save if branch code - std::string branch1 = tok->next()->link()->tokAt(2)->stringifyList(tok->next()->link()->next()->link()); - - // find else branch - const Token *tok1 = tok->next()->link()->next()->link(); + std::string branch1 = scope->classStart->next()->stringifyList(scope->classEnd); // save else branch code - std::string branch2 = tok1->tokAt(3)->stringifyList(tok1->linkAt(2)); + std::string branch2 = scope->classEnd->tokAt(3)->stringifyList(scope->classEnd->linkAt(2)); // check for duplicates if (branch1 == branch2) - duplicateBranchError(tok, tok1->tokAt(2)); + duplicateBranchError(scope->classDef, scope->classEnd->tokAt(1)); } } } @@ -2329,11 +2318,11 @@ namespace { struct FuncFilter { FuncFilter(const Scope *scope, const Token *tok): _scope(scope), _tok(tok) {} - bool operator()(const Function &func) const { - bool matchingFunc = func.type == Function::eFunction && - _tok->str() == func.token->str(); + bool operator()(const Function* func) const { + bool matchingFunc = func->type == Function::eFunction && + _tok->str() == func->token->str(); // either a class function, or a global function with the same name - return (_scope && _scope == func.functionScope && matchingFunc) || + return (_scope && _scope == func->nestedIn && matchingFunc) || (!_scope && matchingFunc); } const Scope *_scope; @@ -2341,7 +2330,7 @@ namespace { }; bool inconclusiveFunctionCall(const SymbolDatabase *symbolDatabase, - const std::list &constFunctions, + const std::list &constFunctions, const ExpressionTokens &tokens) { const Token *start = tokens.start; @@ -2366,7 +2355,7 @@ namespace { // hard coded list of safe, no-side-effect functions if (v == 0 && Token::Match(prev, "strcmp|strncmp|strlen|memcmp|strcasecmp|strncasecmp")) return false; - std::list::const_iterator it = std::find_if(constFunctions.begin(), + std::list::const_iterator it = std::find_if(constFunctions.begin(), constFunctions.end(), FuncFilter(v ? v->type(): 0, prev)); if (it == constFunctions.end()) @@ -2380,7 +2369,7 @@ namespace { class Expressions { public: Expressions(const SymbolDatabase *symbolDatabase, const - std::list &constFunctions) + std::list &constFunctions) : _start(0), _lastTokens(0), _symbolDatabase(symbolDatabase), @@ -2424,39 +2413,29 @@ namespace { const Token *_start; ExpressionTokens *_lastTokens; const SymbolDatabase *_symbolDatabase; - const std::list &_constFunctions; + const std::list &_constFunctions; }; - bool notconst(const Function &func) + bool notconst(const Function* func) { - return !func.isConst; + return !func->isConst; } - void getConstFunctions(const SymbolDatabase *symbolDatabase, std::list &constFunctions) + void getConstFunctions(const SymbolDatabase *symbolDatabase, std::list &constFunctions) { std::list::const_iterator scope; for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { std::list::const_iterator func; // only add const functions that do not have a non-const overloaded version // since it is pretty much impossible to tell which is being called. - typedef std::map > StringFunctionMap; + typedef std::map > StringFunctionMap; StringFunctionMap functionsByName; for (func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - StringFunctionMap::iterator it = functionsByName.find(func->tokenDef->str()); - Scope *currScope = const_cast(&*scope); - if (it == functionsByName.end()) { - std::list tmp; - tmp.push_back(*func); - tmp.back().functionScope = currScope; - functionsByName[func->tokenDef->str()] = tmp; - } else { - it->second.push_back(*func); - it->second.back().functionScope = currScope; - } + functionsByName[func->tokenDef->str()].push_back(&*func); } for (StringFunctionMap::iterator it = functionsByName.begin(); it != functionsByName.end(); ++it) { - std::list::const_iterator nc = std::find_if(it->second.begin(), it->second.end(), notconst); + std::list::const_iterator nc = std::find_if(it->second.begin(), it->second.end(), notconst); if (nc == it->second.end()) { // ok to add all of them constFunctions.splice(constFunctions.end(), it->second); @@ -2467,7 +2446,7 @@ namespace { } -void CheckOther::checkExpressionRange(const std::list &constFunctions, +void CheckOther::checkExpressionRange(const std::list &constFunctions, const Token *start, const Token *end, const std::string &toCheck) @@ -2536,7 +2515,7 @@ void CheckOther::checkExpressionRange(const std::list &constFunctions, } } -void CheckOther::complexDuplicateExpressionCheck(const std::list &constFunctions, +void CheckOther::complexDuplicateExpressionCheck(const std::list &constFunctions, const Token *classStart, const std::string &toCheck, const std::string &alt) @@ -2598,7 +2577,7 @@ void CheckOther::checkDuplicateExpression() const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); std::list::const_iterator scope; - std::list constFunctions; + std::list constFunctions; getConstFunctions(symbolDatabase, constFunctions); for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { diff --git a/lib/checkother.h b/lib/checkother.h index 1b45f95be..8220dde6f 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -397,12 +397,12 @@ private: "* Comparisons of modulo results that are always true/false.\n"; } - void checkExpressionRange(const std::list &constFunctions, + void checkExpressionRange(const std::list &constFunctions, const Token *start, const Token *end, const std::string &toCheck); - void complexDuplicateExpressionCheck(const std::list &constFunctions, + void complexDuplicateExpressionCheck(const std::list &constFunctions, const Token *classStart, const std::string &toCheck, const std::string &alt); diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index faf37307d..375330487 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -217,6 +217,9 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti // save the function name location function.tokenDef = funcStart; + // save the function parent scope + function.nestedIn = scope; + // operator function if (function.tokenDef->str().find("operator") == 0) { function.isOperator = true; @@ -329,12 +332,11 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti scope->functionList.push_back(function); const Token *tok2 = funcStart; - Scope *functionOf = scope; addNewFunction(&scope, &tok2); if (scope) { - scope->functionOf = functionOf; - scope->function = &functionOf->functionList.back(); + scope->functionOf = function.nestedIn; + scope->function = &function.nestedIn->functionList.back(); scope->function->functionScope = scope; } @@ -963,6 +965,7 @@ Function* SymbolDatabase::addGlobalFunctionDecl(Scope*& scope, const Token *argS function.isInline = false; function.hasBody = false; function.type = Function::eFunction; + function.nestedIn = scope; scope->functionList.push_back(function); return &scope->functionList.back(); @@ -1391,6 +1394,15 @@ void SymbolDatabase::printOut(const char *title) const std::cout << " token: " << _tokenizer->list.fileLine(func->token) << std::endl; std::cout << " arg: " << _tokenizer->list.fileLine(func->arg) << std::endl; } + std::cout << " nestedIn: "; + if (func->nestedIn) { + std::cout << func->nestedIn->className << " " << func->nestedIn->type; + if (func->nestedIn->classDef) + std::cout << " " << _tokenizer->list.fileLine(func->nestedIn->classDef) << std::endl; + else + std::cout << std::endl; + } else + std::cout << "Unknown" << std::endl; std::cout << " functionScope: "; if (func->functionScope) { std::cout << func->functionScope->className << " " diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 98548cb12..015bdd250 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -367,6 +367,7 @@ public: token(NULL), arg(NULL), functionScope(NULL), + nestedIn(NULL), type(eFunction), access(Public), hasBody(false), @@ -395,6 +396,7 @@ public: const Token *token; // function name token in implementation const Token *arg; // function argument start '(' Scope *functionScope; // scope of function body + Scope* nestedIn; // Scope the function is declared in std::list argumentList; // argument list Type type; // constructor, destructor, ... AccessControl access; // public/protected/private diff --git a/test/testother.cpp b/test/testother.cpp index 9e409ba10..c89b5d172 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -2158,39 +2158,27 @@ private: } void trac1132() { - std::istringstream code("#include \n" - "class Lock\n" - "{\n" - "public:\n" - " Lock(int i)\n" - " {\n" - " std::cout << \"Lock \" << i << std::endl;\n" - " }\n" - " ~Lock()\n" - " {\n" - " std::cout << \"~Lock\" << std::endl;\n" - " }\n" - "};\n" - "int main()\n" - "{\n" - " Lock(123);\n" - " std::cout << \"hello\" << std::endl;\n" - " return 0;\n" - "}\n" - ); - - errout.str(""); - - Settings settings; - - Tokenizer tokenizer(&settings, this); - tokenizer.tokenize(code, "trac1132.cpp"); - tokenizer.simplifyTokenList(); - - CheckOther checkOther(&tokenizer, &settings, this); - checkOther.checkMisusedScopedObject(); - - ASSERT_EQUALS("[trac1132.cpp:16]: (error) Instance of \"Lock\" object destroyed immediately.\n", errout.str()); + check("#include \n" + "class Lock\n" + "{\n" + "public:\n" + " Lock(int i)\n" + " {\n" + " std::cout << \"Lock \" << i << std::endl;\n" + " }\n" + " ~Lock()\n" + " {\n" + " std::cout << \"~Lock\" << std::endl;\n" + " }\n" + "};\n" + "int main()\n" + "{\n" + " Lock(123);\n" + " std::cout << \"hello\" << std::endl;\n" + " return 0;\n" + "}\n" + ); + ASSERT_EQUALS("[test.cpp:16]: (error) Instance of \"Lock\" object destroyed immediately.\n", errout.str()); } void trac3693() { diff --git a/test/testsymboldatabase.cpp b/test/testsymboldatabase.cpp index 89c2b73e9..1c0eca125 100644 --- a/test/testsymboldatabase.cpp +++ b/test/testsymboldatabase.cpp @@ -496,7 +496,7 @@ private: ASSERT(function && function->token->str() == "func"); ASSERT(function && function->token == tokenizer.tokens()->next()); ASSERT(function && function->hasBody); - ASSERT(function && function->functionScope == scope && scope->function == function); + ASSERT(function && function->functionScope == scope && scope->function == function && function->nestedIn != scope); } } @@ -517,7 +517,7 @@ private: ASSERT(function && function->token->str() == "func"); ASSERT(function && function->token == tokenizer.tokens()->tokAt(4)); ASSERT(function && function->hasBody && function->isInline); - ASSERT(function && function->functionScope == scope && scope->function == function); + ASSERT(function && function->functionScope == scope && scope->function == function && function->nestedIn == db->findScopeByName("Fred")); } } @@ -557,7 +557,7 @@ private: ASSERT(function && function->token->str() == "func"); ASSERT(function && function->token == tokenizer.tokens()->tokAt(12)); ASSERT(function && function->hasBody && !function->isInline); - ASSERT(function && function->functionScope == scope && scope->function == function); + ASSERT(function && function->functionScope == scope && scope->function == function && function->nestedIn == db->findScopeByName("Fred")); } }