diff --git a/cli/cppcheckexecutor.h b/cli/cppcheckexecutor.h index 2b1c6a92b..095675dac 100644 --- a/cli/cppcheckexecutor.h +++ b/cli/cppcheckexecutor.h @@ -57,7 +57,7 @@ public: * given value is returned instead of default 0. * If no errors are found, 0 is returned. */ - virtual int check(int argc, const char* const argv[]); + int check(int argc, const char* const argv[]); /** * Information about progress is directed here. This should be @@ -88,7 +88,7 @@ protected: * Helper function to print out errors. Appends a line change. * @param errmsg String printed to error stream */ - virtual void reportErr(const std::string &errmsg); + void reportErr(const std::string &errmsg); /** * @brief Parse command line args and get settings and file lists diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index a741c64c8..20fdaa469 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -508,6 +508,52 @@ void CheckClass::operatorEqVarError(const Token *tok, const std::string &classna // ClassCheck: Unused private functions //--------------------------------------------------------------------------- +static bool checkBaseFunctionsRec(const Scope* scope, std::list& FuncList, bool& whole) +{ + for (std::vector::const_iterator i = scope->derivedFrom.begin(); i != scope->derivedFrom.end(); ++i) { + if (!i->scope || !i->scope->classStart) { + whole = false; // We need to see the complete definition of the class + return false; + } + for (std::list::const_iterator func = i->scope->functionList.begin(); func != i->scope->functionList.end(); ++func) { + if (func->isVirtual) { // Only virtual functions can be overriden + // Remove function from FuncList. TODO: Handle overloads + bool found = false; + std::list::iterator it = FuncList.begin(); + while (it != FuncList.end()) { + if (func->token->str() == (*it)->str()) { + FuncList.erase(it++); + found = true; + } else + ++it; + } + if (found) + return false; + } + } + if (!checkBaseFunctionsRec(i->scope, FuncList, whole)) + return false; + } + return true; +} + +static bool checkFunctionUsage(const std::string& name, const Scope* scope) +{ + if (!scope) + return true; // Assume its used, if scope is not seen + + for (std::list::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { + if (func->start) { + for (const Token *ftok = func->start; ftok != func->start->link(); ftok = ftok->next()) { + if (ftok->str() == name && ftok->next()->str() == "(") // Function called. TODO: Handle overloads + return true; + } + } + } + + return false; // Unused in this scope +} + void CheckClass::privateFunctions() { if (!_settings->isEnabled("style")) @@ -518,17 +564,11 @@ void CheckClass::privateFunctions() if (_tokenizer->codeWithTemplates()) return; - std::list::const_iterator scope; - - for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { + for (std::list::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) { // only check classes and structures if (!scope->isClassOrStruct()) continue; - // skip checking if there are friends - if (!scope->friendList.empty()) - continue; - // dont check borland classes with properties.. if (Token::findsimplematch(scope->classStart, "; __property ;", scope->classEnd)) continue; @@ -561,63 +601,36 @@ void CheckClass::privateFunctions() } // Bailout for overriden virtual functions of base classes - if (!scope->derivedFrom.empty()) { - for (std::vector::const_iterator i = scope->derivedFrom.begin(); i != scope->derivedFrom.end(); ++i) { - if (!i->scope || !i->scope->classStart) { - whole = false; // We need to see the complete definition of the class - break; - } - for (std::list::const_iterator func = i->scope->functionList.begin(); func != i->scope->functionList.end(); ++func) { - if (func->isVirtual) { // Only virtual functions can be overriden - // Remove function from FuncList. TODO: Handle overloads - std::list::iterator it = FuncList.begin(); - while (it != FuncList.end()) { - if (func->token->str() == (*it)->str()) - FuncList.erase(it++); - else - ++it; - } - } - } - } - } + if (!scope->derivedFrom.empty()) + checkBaseFunctionsRec(&*scope, FuncList, whole); if (!whole) continue; - // Check that all private functions are used.. - for (std::list::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { - if (func->start) { - for (const Token *ftok = func->start; ftok != func->start->link(); ftok = ftok->next()) { - if (Token::Match(ftok, "%var% (")) { - // Remove function from FuncList. TODO: Handle overloads - std::list::iterator it = FuncList.begin(); - while (it != FuncList.end()) { - if (ftok->str() == (*it)->str()) - FuncList.erase(it++); - else - ++it; - } - } + while (!FuncList.empty()) { + // Check that all private functions are used + bool used = checkFunctionUsage(FuncList.front()->str(), &*scope); // Usage in this class + // Check in friend classes + for (std::list::const_iterator i = scope->friendList.begin(); !used && i != scope->friendList.end(); ++i) + used = checkFunctionUsage(FuncList.front()->str(), i->scope); + + if (!used) { + // Final check; check if the function pointer is used somewhere.. + const std::string _pattern("return|throw|(|)|,|= &|" + FuncList.front()->str()); + + // or if the function address is used somewhere... + // eg. sigc::mem_fun(this, &className::classFunction) + const std::string _pattern2("& " + scope->className + " :: " + FuncList.front()->str()); + const std::string methodAsArgument("(|, " + scope->className + " :: " + FuncList.front()->str() + " ,|)"); + const std::string methodAssigned("%var% = &| " + scope->className + " :: " + FuncList.front()->str()); + + if (!Token::findmatch(_tokenizer->tokens(), _pattern.c_str()) && + !Token::findmatch(_tokenizer->tokens(), _pattern2.c_str()) && + !Token::findmatch(_tokenizer->tokens(), methodAsArgument.c_str()) && + !Token::findmatch(_tokenizer->tokens(), methodAssigned.c_str())) { + unusedPrivateFunctionError(FuncList.front(), scope->className, FuncList.front()->str()); } } - } - while (!FuncList.empty()) { - // Final check; check if the function pointer is used somewhere.. - const std::string _pattern("return|throw|(|)|,|= &|" + FuncList.front()->str()); - - // or if the function address is used somewhere... - // eg. sigc::mem_fun(this, &className::classFunction) - const std::string _pattern2("& " + scope->className + " :: " + FuncList.front()->str()); - const std::string methodAsArgument("(|, " + scope->className + " :: " + FuncList.front()->str() + " ,|)"); - const std::string methodAssigned("%var% = &| " + scope->className + " :: " + FuncList.front()->str()); - - if (!Token::findmatch(_tokenizer->tokens(), _pattern.c_str()) && - !Token::findmatch(_tokenizer->tokens(), _pattern2.c_str()) && - !Token::findmatch(_tokenizer->tokens(), methodAsArgument.c_str()) && - !Token::findmatch(_tokenizer->tokens(), methodAssigned.c_str())) { - unusedPrivateFunctionError(FuncList.front(), scope->className, FuncList.front()->str()); - } FuncList.pop_front(); } } diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index 4a587339d..3e2f90ad8 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -67,28 +67,24 @@ const char * CppCheck::extraVersion() unsigned int CppCheck::check(const std::string &path) { - _filename = path; - return processFile(); + return processFile(path); } unsigned int CppCheck::check(const std::string &path, const std::string &content) { - _filename = path; _fileContent = content; - const unsigned int retval = processFile(); + const unsigned int retval = processFile(path); _fileContent.clear(); return retval; } -std::string CppCheck::replaceAll(std::string code, const std::string &from, const std::string &to) const +void CppCheck::replaceAll(std::string& code, const std::string &from, const std::string &to) { size_t pos = 0; while ((pos = code.find(from, pos)) != std::string::npos) { code.replace(pos, from.length(), to); pos += to.length(); } - - return code; } bool CppCheck::findError(std::string code, const char FileName[]) @@ -126,8 +122,8 @@ bool CppCheck::findError(std::string code, const char FileName[]) // Add '\n' so that "\n#file" on first line would be found code = "// " + error + "\n" + code; - code = replaceAll(code, "\n#file", "\n// #file"); - code = replaceAll(code, "\n#endfile", "\n// #endfile"); + replaceAll(code, "\n#file", "\n// #file"); + replaceAll(code, "\n#endfile", "\n// #endfile"); // We have reduced the code as much as we can. Print out // the code and quit. @@ -138,23 +134,23 @@ bool CppCheck::findError(std::string code, const char FileName[]) return true; } -unsigned int CppCheck::processFile() +unsigned int CppCheck::processFile(const std::string& filename) { exitcode = 0; // only show debug warnings for C/C++ source files (don't fix // debug warnings for java/c#/etc files) - if (!Path::acceptFile(_filename)) + if (!Path::acceptFile(filename)) _settings.debugwarnings = false; // TODO: Should this be moved out to its own function so all the files can be // analysed before any files are checked? if (_settings.test_2_pass && _settings._jobs == 1) { - const std::string printname = Path::toNativeSeparators(_filename); + const std::string printname = Path::toNativeSeparators(filename); reportOut("Analysing " + printname + "..."); - std::ifstream f(_filename.c_str()); - analyseFile(f, _filename); + std::ifstream f(filename.c_str()); + analyseFile(f, filename); } _errout.str(""); @@ -163,7 +159,7 @@ unsigned int CppCheck::processFile() return exitcode; if (_settings._errorsOnly == false) { - std::string fixedpath(_filename); + std::string fixedpath(filename); fixedpath = Path::simplifyPath(fixedpath.c_str()); fixedpath = Path::toNativeSeparators(fixedpath); _errorLogger.reportOut(std::string("Checking ") + fixedpath + std::string("...")); @@ -177,12 +173,12 @@ unsigned int CppCheck::processFile() if (!_fileContent.empty()) { // File content was given as a string std::istringstream iss(_fileContent); - preprocessor.preprocess(iss, filedata, configurations, _filename, _settings._includePaths); + preprocessor.preprocess(iss, filedata, configurations, filename, _settings._includePaths); } else { // Only file name was given, read the content from file - std::ifstream fin(_filename.c_str()); + std::ifstream fin(filename.c_str()); Timer t("Preprocessor::preprocess", _settings._showtime, &S_timerResults); - preprocessor.preprocess(fin, filedata, configurations, _filename, _settings._includePaths); + preprocessor.preprocess(fin, filedata, configurations, filename, _settings._includePaths); } if (_settings.checkConfiguration) { @@ -200,7 +196,7 @@ unsigned int CppCheck::processFile() // was used. if (!_settings._force && checkCount >= _settings._maxConfigs) { - const std::string fixedpath = Path::toNativeSeparators(_filename); + const std::string fixedpath = Path::toNativeSeparators(filename); ErrorLogger::ErrorMessage::FileLocation location; location.setfile(fixedpath); std::list loclist; @@ -225,12 +221,12 @@ unsigned int CppCheck::processFile() cfg = *it; Timer t("Preprocessor::getcode", _settings._showtime, &S_timerResults); - const std::string codeWithoutCfg = preprocessor.getcode(filedata, *it, _filename); + const std::string codeWithoutCfg = preprocessor.getcode(filedata, *it, filename); t.Stop(); // If only errors are printed, print filename after the check if (_settings._errorsOnly == false && it != configurations.begin()) { - std::string fixedpath = Path::simplifyPath(_filename.c_str()); + std::string fixedpath = Path::simplifyPath(filename.c_str()); fixedpath = Path::toNativeSeparators(fixedpath); _errorLogger.reportOut(std::string("Checking ") + fixedpath + ": " + cfg + std::string("...")); } @@ -238,23 +234,23 @@ unsigned int CppCheck::processFile() const std::string &appendCode = _settings.append(); if (_settings.debugFalsePositive) { - if (findError(codeWithoutCfg + appendCode, _filename.c_str())) { + if (findError(codeWithoutCfg + appendCode, filename.c_str())) { return exitcode; } } else { - checkFile(codeWithoutCfg + appendCode, _filename.c_str()); + checkFile(codeWithoutCfg + appendCode, filename.c_str()); } ++checkCount; } } catch (std::runtime_error &e) { // Exception was thrown when checking this file.. - const std::string fixedpath = Path::toNativeSeparators(_filename); + const std::string fixedpath = Path::toNativeSeparators(filename); _errorLogger.reportOut("Bailing out from checking " + fixedpath + ": " + e.what()); } if (!_settings._errorsOnly) - reportUnmatchedSuppressions(_settings.nomsg.getUnmatchedLocalSuppressions(_filename)); + reportUnmatchedSuppressions(_settings.nomsg.getUnmatchedLocalSuppressions(filename)); _errorList.clear(); return exitcode; diff --git a/lib/cppcheck.h b/lib/cppcheck.h index 4f5acd4a7..971574ce4 100644 --- a/lib/cppcheck.h +++ b/lib/cppcheck.h @@ -141,7 +141,7 @@ public: private: /** @brief Process one file. */ - unsigned int processFile(); + unsigned int processFile(const std::string& filename); /** @brief Check file */ void checkFile(const std::string &code, const char FileName[]); @@ -173,14 +173,13 @@ private: * @brief Replace "from" strings with "to" strings in "code" * and return it. */ - std::string replaceAll(std::string code, const std::string &from, const std::string &to) const; + static void replaceAll(std::string& code, const std::string &from, const std::string &to); unsigned int exitcode; std::list _errorList; std::ostringstream _errout; Settings _settings; bool _useGlobalSuppressions; - std::string _filename; std::string _fileContent; std::set _dependencies; diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 1f33c2136..9299212f8 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -363,7 +363,7 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti Scope::FriendInfo friendInfo; friendInfo.name = tok->strAt(1) == "class" ? tok->strAt(2) : tok->strAt(1); - /** @todo fill this in later after parsing is complete */ + // fill this in after parsing is complete friendInfo.scope = 0; scope->friendList.push_back(friendInfo); @@ -546,6 +546,23 @@ SymbolDatabase::SymbolDatabase(const Tokenizer *tokenizer, const Settings *setti } } + // fill in friend info + for (it = scopeList.begin(); it != scopeList.end(); ++it) { + for (std::list::iterator i = it->friendList.begin(); i != it->friendList.end(); ++i) { + for (std::list::iterator j = scopeList.begin(); j != scopeList.end(); ++j) { + // check scope for match + const Scope *scope = j->findQualifiedScope(i->name); + + // found match? + if (scope && scope->isClassOrStruct()) { + // set found scope + i->scope = const_cast(scope); + break; + } + } + } + } + // fill in variable info for (it = scopeList.begin(); it != scopeList.end(); ++it) { scope = &(*it); @@ -1651,8 +1668,6 @@ AccessControl Scope::defaultAccess() const default: return Local; } - - return Public; } // Get variable list.. @@ -2032,6 +2047,17 @@ const Function *SymbolDatabase::findFunctionByToken(const Token *tok) const //--------------------------------------------------------------------------- +const Scope* SymbolDatabase::findScopeByName(const std::string& name) const +{ + for (std::list::const_iterator it = scopeList.begin(); it != scopeList.end(); ++it) { + if (it->className == name) + return &*it; + } + return 0; +} + +//--------------------------------------------------------------------------- + Scope * Scope::findInNestedList(const std::string & name) { std::list::iterator it; diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 1cf541281..2e22942ce 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -552,6 +552,8 @@ public: const Function *findFunctionByToken(const Token *tok) const; + const Scope* findScopeByName(const std::string& name) const; + bool argsMatch(const Scope *info, const Token *first, const Token *second, const std::string &path, unsigned int depth) const; bool isClassOrStruct(const std::string &type) const { diff --git a/lib/templatesimplifier.h b/lib/templatesimplifier.h index 71f92dbed..4e257b14f 100644 --- a/lib/templatesimplifier.h +++ b/lib/templatesimplifier.h @@ -39,7 +39,7 @@ class Settings; class TemplateSimplifier { public: TemplateSimplifier(); - virtual ~TemplateSimplifier(); + ~TemplateSimplifier(); /** * Used after simplifyTemplates to perform a little cleanup. diff --git a/lib/tokenize.h b/lib/tokenize.h index c2a7121cd..c79664578 100644 --- a/lib/tokenize.h +++ b/lib/tokenize.h @@ -47,7 +47,7 @@ private: public: Tokenizer(); Tokenizer(const Settings * settings, ErrorLogger *errorLogger); - virtual ~Tokenizer(); + ~Tokenizer(); /** Returns the source file path. e.g. "file.cpp" */ std::string getSourceFilePath() const; @@ -204,7 +204,7 @@ public: /** * get error messages that the tokenizer generate */ - virtual void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const; + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const; /** Simplify assignment in function call "f(x=g());" => "x=g();f(x);" */ diff --git a/test/testsymboldatabase.cpp b/test/testsymboldatabase.cpp index 31df5a199..5a9c9a9a3 100644 --- a/test/testsymboldatabase.cpp +++ b/test/testsymboldatabase.cpp @@ -113,6 +113,8 @@ private: TEST_CASE(hasMissingInlineClassFunctionReturningFunctionPointer); TEST_CASE(hasClassFunctionReturningFunctionPointer); + TEST_CASE(classWithFriend); + TEST_CASE(parseFunctionCorrect); TEST_CASE(hasGlobalVariables1); @@ -631,6 +633,25 @@ private: } } + void classWithFriend() { + GET_SYMBOL_DB("class Foo {}; class Bar1 { friend class Foo; }; class Bar2 { friend Foo; };") + // 3 scopes: Global, 3 classes + ASSERT(db && db->scopeList.size() == 4); + if (db) { + const Scope* foo = db->findScopeByName("Foo"); + ASSERT(foo != 0); + const Scope* bar1 = db->findScopeByName("Bar1"); + ASSERT(bar1 != 0); + const Scope* bar2 = db->findScopeByName("Bar2"); + ASSERT(bar2 != 0); + + if (foo && bar1 && bar2) { + ASSERT(bar1->friendList.size() == 1 && bar1->friendList.front().name == "Foo" && bar1->friendList.front().scope == foo); + ASSERT(bar2->friendList.size() == 1 && bar2->friendList.front().name == "Foo" && bar2->friendList.front().scope == foo); + } + } + } + void parseFunctionCorrect() { // ticket 3188 - "if" statement parsed as function GET_SYMBOL_DB("void func(i) int i; { if (i == 1) return; }\n") diff --git a/test/testunusedprivfunc.cpp b/test/testunusedprivfunc.cpp index b12802c2d..033775bf1 100644 --- a/test/testunusedprivfunc.cpp +++ b/test/testunusedprivfunc.cpp @@ -423,16 +423,53 @@ private: " void bar() {}\n" // Don't skip if no function is overriden "};"); ASSERT_EQUALS("[test.cpp:9]: (style) Unused private function 'derived::bar'\n", errout.str()); + + check("class Base {\n" + "private:\n" + " virtual void func() = 0;\n" + "public:\n" + " void public_func() {\n" + " func();\n" + " };\n" + "};\n" + "\n" + "class Derived1: public Base {\n" + "private:\n" + " void func() {}\n" + "};\n" + "class Derived2: public Derived1 {\n" + "private:\n" + " void func() {}\n" + "};"); + ASSERT_EQUALS("", errout.str()); } void friendClass() { // ticket #2459 - friend class check("class Foo {\n" + "private:\n" + " friend Bar;\n" // Unknown friend class + " void f() { }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + check("struct Bar {\n" + " void g() { f(); }\n" // Friend class seen, but f() used in it + "};\n" + "class Foo {\n" "private:\n" " friend Bar;\n" " void f() { }\n" "};"); ASSERT_EQUALS("", errout.str()); + + check("class Bar {\n" // Friend class seen, f() not used in it + "};\n" + "class Foo {\n" + " friend Bar;\n" + " void f() { }\n" + "};"); + ASSERT_EQUALS("[test.cpp:5]: (style) Unused private function 'Foo::f'\n", errout.str()); } void borland() {