Improved unused private function check:

- Fixed #3628
- Added support for friend
Improved symbol database:
- friend scopes are now set
- Added findScopeByName function
Refactorizations:
- Removed some unnecessary "virtual" keywords
- Removed unnecessary _filename member variable, pass it as argument instead
- Made CppCheck::replaceAll static, since it is independant from a specific CppCheck instance, Pass string to be modified by reference

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

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<const Token*>& FuncList, bool& whole)
+{
+    for (std::vector<Scope::BaseInfo>::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<Function>::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<const Token*>::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<Function>::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<Scope>::const_iterator scope;
+    for (std::list<Scope>::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
-
-    for (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,48 +601,19 @@ void CheckClass::privateFunctions()
         }
 
         // Bailout for overriden virtual functions of base classes
-        if (!scope->derivedFrom.empty()) {
+        if (!scope->derivedFrom.empty())
-            for (std::vector<Scope::BaseInfo>::const_iterator i = scope->derivedFrom.begin(); i != scope->derivedFrom.end(); ++i) {
+            checkBaseFunctionsRec(&*scope, FuncList, whole);
-                if (!i->scope || !i->scope->classStart) {
-                    whole = false; // We need to see the complete definition of the class
-                    break;
-                }
-                for (std::list<Function>::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<const Token *>::iterator it = FuncList.begin();
-                        while (it != FuncList.end()) {
-                            if (func->token->str() == (*it)->str())
-                                FuncList.erase(it++);
-                            else
-                                ++it;
-                        }
-                    }
-                }
-            }
-        }
         if (!whole)
             continue;
 
-        // Check that all private functions are used..
-        for (std::list<Function>::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<const Token *>::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<Scope::FriendInfo>::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());
 
@@ -618,6 +629,8 @@ void CheckClass::privateFunctions()
                     !Token::findmatch(_tokenizer->tokens(), methodAssigned.c_str())) {
                     unusedPrivateFunctionError(FuncList.front(), scope->className, FuncList.front()->str());
                 }
+            }
+
             FuncList.pop_front();
         }
     }

lib/cppcheck.cpp

@@ -67,28 +67,24 @@ const char * CppCheck::extraVersion()
 
 unsigned int CppCheck::check(const std::string &path)
 {
-    _filename = path;
+    return processFile(path);
-    return processFile();
 }
 
 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");
+        replaceAll(code, "\n#file", "\n// #file");
-        code = replaceAll(code, "\n#endfile", "\n// #endfile");
+        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());
+        std::ifstream f(filename.c_str());
-        analyseFile(f, _filename);
+        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<ErrorLogger::ErrorMessage::FileLocation> 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;

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<std::string> _errorList;
     std::ostringstream _errout;
     Settings _settings;
     bool _useGlobalSuppressions;
-    std::string _filename;
     std::string _fileContent;
     std::set<std::string> _dependencies;
 

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<Scope::FriendInfo>::iterator i = it->friendList.begin(); i != it->friendList.end(); ++i) {
+            for (std::list<Scope>::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 *>(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<Scope>::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<Scope *>::iterator it;

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 {

lib/templatesimplifier.h

@@ -39,7 +39,7 @@ class Settings;
 class TemplateSimplifier {
 public:
     TemplateSimplifier();
-    virtual ~TemplateSimplifier();
+    ~TemplateSimplifier();
 
     /**
      * Used after simplifyTemplates to perform a little cleanup.

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);"
      */

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")

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() {