Skip to content

Commit 9a5f660

Browse files
PKEuSPKEuS
committed
Improved unused private function check:
- Fixed cppcheck-opensource#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
1 parent 9f42ce9 commit 9a5f660

10 files changed

Lines changed: 188 additions & 94 deletions

‎cli/cppcheckexecutor.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ class CppCheckExecutor : public ErrorLogger {
5757
* given value is returned instead of default 0.
5858
* If no errors are found, 0 is returned.
5959
*/
60-
virtual int check(int argc, const char* const argv[]);
60+
int check(int argc, const char* const argv[]);
6161

6262
/**
6363
* Information about progress is directed here. This should be
@@ -88,7 +88,7 @@ class CppCheckExecutor : public ErrorLogger {
8888
* Helper function to print out errors. Appends a line change.
8989
* @param errmsg String printed to error stream
9090
*/
91-
virtual void reportErr(const std::string &errmsg);
91+
void reportErr(const std::string &errmsg);
9292

9393
/**
9494
* @brief Parse command line args and get settings and file lists

‎lib/checkclass.cpp‎

Lines changed: 71 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,52 @@ void CheckClass::operatorEqVarError(const Token *tok, const std::string &classna
508508
// ClassCheck: Unused private functions
509509
//---------------------------------------------------------------------------
510510

511+
static bool checkBaseFunctionsRec(const Scope* scope, std::list<const Token*>& FuncList, bool& whole)
512+
{
513+
for (std::vector<Scope::BaseInfo>::const_iterator i = scope->derivedFrom.begin(); i != scope->derivedFrom.end(); ++i) {
514+
if (!i->scope || !i->scope->classStart) {
515+
whole = false; // We need to see the complete definition of the class
516+
return false;
517+
}
518+
for (std::list<Function>::const_iterator func = i->scope->functionList.begin(); func != i->scope->functionList.end(); ++func) {
519+
if (func->isVirtual) { // Only virtual functions can be overriden
520+
// Remove function from FuncList. TODO: Handle overloads
521+
bool found = false;
522+
std::list<const Token*>::iterator it = FuncList.begin();
523+
while (it != FuncList.end()) {
524+
if (func->token->str() == (*it)->str()) {
525+
FuncList.erase(it++);
526+
found = true;
527+
} else
528+
++it;
529+
}
530+
if (found)
531+
return false;
532+
}
533+
}
534+
if (!checkBaseFunctionsRec(i->scope, FuncList, whole))
535+
return false;
536+
}
537+
return true;
538+
}
539+
540+
static bool checkFunctionUsage(const std::string& name, const Scope* scope)
541+
{
542+
if (!scope)
543+
return true; // Assume its used, if scope is not seen
544+
545+
for (std::list<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
546+
if (func->start) {
547+
for (const Token *ftok = func->start; ftok != func->start->link(); ftok = ftok->next()) {
548+
if (ftok->str() == name && ftok->next()->str() == "(") // Function called. TODO: Handle overloads
549+
return true;
550+
}
551+
}
552+
}
553+
554+
return false; // Unused in this scope
555+
}
556+
511557
void CheckClass::privateFunctions()
512558
{
513559
if (!_settings->isEnabled("style"))
@@ -518,17 +564,11 @@ void CheckClass::privateFunctions()
518564
if (_tokenizer->codeWithTemplates())
519565
return;
520566

521-
std::list<Scope>::const_iterator scope;
522-
523-
for (scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
567+
for (std::list<Scope>::const_iterator scope = symbolDatabase->scopeList.begin(); scope != symbolDatabase->scopeList.end(); ++scope) {
524568
// only check classes and structures
525569
if (!scope->isClassOrStruct())
526570
continue;
527571

528-
// skip checking if there are friends
529-
if (!scope->friendList.empty())
530-
continue;
531-
532572
// dont check borland classes with properties..
533573
if (Token::findsimplematch(scope->classStart, "; __property ;", scope->classEnd))
534574
continue;
@@ -561,63 +601,36 @@ void CheckClass::privateFunctions()
561601
}
562602

563603
// Bailout for overriden virtual functions of base classes
564-
if (!scope->derivedFrom.empty()) {
565-
for (std::vector<Scope::BaseInfo>::const_iterator i = scope->derivedFrom.begin(); i != scope->derivedFrom.end(); ++i) {
566-
if (!i->scope || !i->scope->classStart) {
567-
whole = false; // We need to see the complete definition of the class
568-
break;
569-
}
570-
for (std::list<Function>::const_iterator func = i->scope->functionList.begin(); func != i->scope->functionList.end(); ++func) {
571-
if (func->isVirtual) { // Only virtual functions can be overriden
572-
// Remove function from FuncList. TODO: Handle overloads
573-
std::list<const Token *>::iterator it = FuncList.begin();
574-
while (it != FuncList.end()) {
575-
if (func->token->str() == (*it)->str())
576-
FuncList.erase(it++);
577-
else
578-
++it;
579-
}
580-
}
581-
}
582-
}
583-
}
604+
if (!scope->derivedFrom.empty())
605+
checkBaseFunctionsRec(&*scope, FuncList, whole);
584606
if (!whole)
585607
continue;
586608

587-
// Check that all private functions are used..
588-
for (std::list<Function>::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) {
589-
if (func->start) {
590-
for (const Token *ftok = func->start; ftok != func->start->link(); ftok = ftok->next()) {
591-
if (Token::Match(ftok, "%var% (")) {
592-
// Remove function from FuncList. TODO: Handle overloads
593-
std::list<const Token *>::iterator it = FuncList.begin();
594-
while (it != FuncList.end()) {
595-
if (ftok->str() == (*it)->str())
596-
FuncList.erase(it++);
597-
else
598-
++it;
599-
}
600-
}
609+
while (!FuncList.empty()) {
610+
// Check that all private functions are used
611+
bool used = checkFunctionUsage(FuncList.front()->str(), &*scope); // Usage in this class
612+
// Check in friend classes
613+
for (std::list<Scope::FriendInfo>::const_iterator i = scope->friendList.begin(); !used && i != scope->friendList.end(); ++i)
614+
used = checkFunctionUsage(FuncList.front()->str(), i->scope);
615+
616+
if (!used) {
617+
// Final check; check if the function pointer is used somewhere..
618+
const std::string _pattern("return|throw|(|)|,|= &|" + FuncList.front()->str());
619+
620+
// or if the function address is used somewhere...
621+
// eg. sigc::mem_fun(this, &className::classFunction)
622+
const std::string _pattern2("& " + scope->className + " :: " + FuncList.front()->str());
623+
const std::string methodAsArgument("(|, " + scope->className + " :: " + FuncList.front()->str() + " ,|)");
624+
const std::string methodAssigned("%var% = &| " + scope->className + " :: " + FuncList.front()->str());
625+
626+
if (!Token::findmatch(_tokenizer->tokens(), _pattern.c_str()) &&
627+
!Token::findmatch(_tokenizer->tokens(), _pattern2.c_str()) &&
628+
!Token::findmatch(_tokenizer->tokens(), methodAsArgument.c_str()) &&
629+
!Token::findmatch(_tokenizer->tokens(), methodAssigned.c_str())) {
630+
unusedPrivateFunctionError(FuncList.front(), scope->className, FuncList.front()->str());
601631
}
602632
}
603-
}
604633

605-
while (!FuncList.empty()) {
606-
// Final check; check if the function pointer is used somewhere..
607-
const std::string _pattern("return|throw|(|)|,|= &|" + FuncList.front()->str());
608-
609-
// or if the function address is used somewhere...
610-
// eg. sigc::mem_fun(this, &className::classFunction)
611-
const std::string _pattern2("& " + scope->className + " :: " + FuncList.front()->str());
612-
const std::string methodAsArgument("(|, " + scope->className + " :: " + FuncList.front()->str() + " ,|)");
613-
const std::string methodAssigned("%var% = &| " + scope->className + " :: " + FuncList.front()->str());
614-
615-
if (!Token::findmatch(_tokenizer->tokens(), _pattern.c_str()) &&
616-
!Token::findmatch(_tokenizer->tokens(), _pattern2.c_str()) &&
617-
!Token::findmatch(_tokenizer->tokens(), methodAsArgument.c_str()) &&
618-
!Token::findmatch(_tokenizer->tokens(), methodAssigned.c_str())) {
619-
unusedPrivateFunctionError(FuncList.front(), scope->className, FuncList.front()->str());
620-
}
621634
FuncList.pop_front();
622635
}
623636
}

‎lib/cppcheck.cpp‎

Lines changed: 21 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -67,28 +67,24 @@ const char * CppCheck::extraVersion()
6767

6868
unsigned int CppCheck::check(const std::string &path)
6969
{
70-
_filename = path;
71-
return processFile();
70+
return processFile(path);
7271
}
7372

7473
unsigned int CppCheck::check(const std::string &path, const std::string &content)
7574
{
76-
_filename = path;
7775
_fileContent = content;
78-
const unsigned int retval = processFile();
76+
const unsigned int retval = processFile(path);
7977
_fileContent.clear();
8078
return retval;
8179
}
8280

83-
std::string CppCheck::replaceAll(std::string code, const std::string &from, const std::string &to) const
81+
void CppCheck::replaceAll(std::string& code, const std::string &from, const std::string &to)
8482
{
8583
size_t pos = 0;
8684
while ((pos = code.find(from, pos)) != std::string::npos) {
8785
code.replace(pos, from.length(), to);
8886
pos += to.length();
8987
}
90-
91-
return code;
9288
}
9389

9490
bool CppCheck::findError(std::string code, const char FileName[])
@@ -126,8 +122,8 @@ bool CppCheck::findError(std::string code, const char FileName[])
126122

127123
// Add '\n' so that "\n#file" on first line would be found
128124
code = "// " + error + "\n" + code;
129-
code = replaceAll(code, "\n#file", "\n// #file");
130-
code = replaceAll(code, "\n#endfile", "\n// #endfile");
125+
replaceAll(code, "\n#file", "\n// #file");
126+
replaceAll(code, "\n#endfile", "\n// #endfile");
131127

132128
// We have reduced the code as much as we can. Print out
133129
// the code and quit.
@@ -138,23 +134,23 @@ bool CppCheck::findError(std::string code, const char FileName[])
138134
return true;
139135
}
140136

141-
unsigned int CppCheck::processFile()
137+
unsigned int CppCheck::processFile(const std::string& filename)
142138
{
143139
exitcode = 0;
144140

145141
// only show debug warnings for C/C++ source files (don't fix
146142
// debug warnings for java/c#/etc files)
147-
if (!Path::acceptFile(_filename))
143+
if (!Path::acceptFile(filename))
148144
_settings.debugwarnings = false;
149145

150146
// TODO: Should this be moved out to its own function so all the files can be
151147
// analysed before any files are checked?
152148
if (_settings.test_2_pass && _settings._jobs == 1) {
153-
const std::string printname = Path::toNativeSeparators(_filename);
149+
const std::string printname = Path::toNativeSeparators(filename);
154150
reportOut("Analysing " + printname + "...");
155151

156-
std::ifstream f(_filename.c_str());
157-
analyseFile(f, _filename);
152+
std::ifstream f(filename.c_str());
153+
analyseFile(f, filename);
158154
}
159155

160156
_errout.str("");
@@ -163,7 +159,7 @@ unsigned int CppCheck::processFile()
163159
return exitcode;
164160

165161
if (_settings._errorsOnly == false) {
166-
std::string fixedpath(_filename);
162+
std::string fixedpath(filename);
167163
fixedpath = Path::simplifyPath(fixedpath.c_str());
168164
fixedpath = Path::toNativeSeparators(fixedpath);
169165
_errorLogger.reportOut(std::string("Checking ") + fixedpath + std::string("..."));
@@ -177,12 +173,12 @@ unsigned int CppCheck::processFile()
177173
if (!_fileContent.empty()) {
178174
// File content was given as a string
179175
std::istringstream iss(_fileContent);
180-
preprocessor.preprocess(iss, filedata, configurations, _filename, _settings._includePaths);
176+
preprocessor.preprocess(iss, filedata, configurations, filename, _settings._includePaths);
181177
} else {
182178
// Only file name was given, read the content from file
183-
std::ifstream fin(_filename.c_str());
179+
std::ifstream fin(filename.c_str());
184180
Timer t("Preprocessor::preprocess", _settings._showtime, &S_timerResults);
185-
preprocessor.preprocess(fin, filedata, configurations, _filename, _settings._includePaths);
181+
preprocessor.preprocess(fin, filedata, configurations, filename, _settings._includePaths);
186182
}
187183

188184
if (_settings.checkConfiguration) {
@@ -200,7 +196,7 @@ unsigned int CppCheck::processFile()
200196
// was used.
201197
if (!_settings._force && checkCount >= _settings._maxConfigs) {
202198

203-
const std::string fixedpath = Path::toNativeSeparators(_filename);
199+
const std::string fixedpath = Path::toNativeSeparators(filename);
204200
ErrorLogger::ErrorMessage::FileLocation location;
205201
location.setfile(fixedpath);
206202
std::list<ErrorLogger::ErrorMessage::FileLocation> loclist;
@@ -225,36 +221,36 @@ unsigned int CppCheck::processFile()
225221

226222
cfg = *it;
227223
Timer t("Preprocessor::getcode", _settings._showtime, &S_timerResults);
228-
const std::string codeWithoutCfg = preprocessor.getcode(filedata, *it, _filename);
224+
const std::string codeWithoutCfg = preprocessor.getcode(filedata, *it, filename);
229225
t.Stop();
230226

231227
// If only errors are printed, print filename after the check
232228
if (_settings._errorsOnly == false && it != configurations.begin()) {
233-
std::string fixedpath = Path::simplifyPath(_filename.c_str());
229+
std::string fixedpath = Path::simplifyPath(filename.c_str());
234230
fixedpath = Path::toNativeSeparators(fixedpath);
235231
_errorLogger.reportOut(std::string("Checking ") + fixedpath + ": " + cfg + std::string("..."));
236232
}
237233

238234
const std::string &appendCode = _settings.append();
239235

240236
if (_settings.debugFalsePositive) {
241-
if (findError(codeWithoutCfg + appendCode, _filename.c_str())) {
237+
if (findError(codeWithoutCfg + appendCode, filename.c_str())) {
242238
return exitcode;
243239
}
244240
} else {
245-
checkFile(codeWithoutCfg + appendCode, _filename.c_str());
241+
checkFile(codeWithoutCfg + appendCode, filename.c_str());
246242
}
247243

248244
++checkCount;
249245
}
250246
} catch (std::runtime_error &e) {
251247
// Exception was thrown when checking this file..
252-
const std::string fixedpath = Path::toNativeSeparators(_filename);
248+
const std::string fixedpath = Path::toNativeSeparators(filename);
253249
_errorLogger.reportOut("Bailing out from checking " + fixedpath + ": " + e.what());
254250
}
255251

256252
if (!_settings._errorsOnly)
257-
reportUnmatchedSuppressions(_settings.nomsg.getUnmatchedLocalSuppressions(_filename));
253+
reportUnmatchedSuppressions(_settings.nomsg.getUnmatchedLocalSuppressions(filename));
258254

259255
_errorList.clear();
260256
return exitcode;

‎lib/cppcheck.h‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ class CppCheck : ErrorLogger {
141141
private:
142142

143143
/** @brief Process one file. */
144-
unsigned int processFile();
144+
unsigned int processFile(const std::string& filename);
145145

146146
/** @brief Check file */
147147
void checkFile(const std::string &code, const char FileName[]);
@@ -173,14 +173,13 @@ class CppCheck : ErrorLogger {
173173
* @brief Replace "from" strings with "to" strings in "code"
174174
* and return it.
175175
*/
176-
std::string replaceAll(std::string code, const std::string &from, const std::string &to) const;
176+
static void replaceAll(std::string& code, const std::string &from, const std::string &to);
177177

178178
unsigned int exitcode;
179179
std::list<std::string> _errorList;
180180
std::ostringstream _errout;
181181
Settings _settings;
182182
bool _useGlobalSuppressions;
183-
std::string _filename;
184183
std::string _fileContent;
185184
std::set<std::string> _dependencies;
186185

0 commit comments

Comments
 (0)