From 8c90edb70ce9db14e0edc9000b9f525d60e7cc5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20St=C3=B6neberg?= Date: Tue, 27 Feb 2024 16:51:04 +0100 Subject: [PATCH] moved global `CheckUnusedFunctions` instance to `CppCheck` / cleaned up `CheckUnusedFunctions` interface (#6013) --- lib/checkunusedfunctions.cpp | 61 ++++++++++--------- lib/checkunusedfunctions.h | 14 ++--- lib/cppcheck.cpp | 37 ++++++++--- lib/cppcheck.h | 3 + test/cli/unusedFunction/3.c | 2 +- test/cli/unusedFunction/4.c | 7 +++ .../unusedFunction/unusedFunction.cppcheck | 1 + test/cli/unused_function_test.py | 35 +++++------ test/testprocessexecutor.cpp | 2 - test/testsingleexecutor.cpp | 2 - test/testsuppressions.cpp | 6 -- test/testthreadexecutor.cpp | 2 - test/testunusedfunctions.cpp | 8 +-- 13 files changed, 96 insertions(+), 84 deletions(-) create mode 100644 test/cli/unusedFunction/4.c diff --git a/lib/checkunusedfunctions.cpp b/lib/checkunusedfunctions.cpp index 33f3812c9b3..63ed6b78ac5 100644 --- a/lib/checkunusedfunctions.cpp +++ b/lib/checkunusedfunctions.cpp @@ -49,11 +49,6 @@ namespace CTU { //--------------------------------------------------------------------------- -// Register this check class -namespace { - CheckUnusedFunctions instance; -} - static const CWE CWE561(561U); // Dead Code static std::string stripTemplateParameters(const std::string& funcName) { @@ -68,14 +63,10 @@ static std::string stripTemplateParameters(const std::string& funcName) { // FUNCTION USAGE - Check for unused functions etc //--------------------------------------------------------------------------- -void CheckUnusedFunctions::clear() +void CheckUnusedFunctions::parseTokens(const Tokenizer &tokenizer, const Settings &settings) { - instance.mFunctions.clear(); - instance.mFunctionCalls.clear(); -} + const char * const FileName = tokenizer.list.getFiles().front().c_str(); -void CheckUnusedFunctions::parseTokens(const Tokenizer &tokenizer, const char FileName[], const Settings &settings) -{ const bool doMarkup = settings.library.markupFile(FileName); // Function declarations.. @@ -100,6 +91,7 @@ void CheckUnusedFunctions::parseTokens(const Tokenizer &tokenizer, const char Fi if (!usage.lineNumber) usage.lineNumber = func->token->linenr(); + // TODO: why always overwrite this but not the filename and line? usage.fileIndex = func->token->fileIndex(); const std::string& fileName = tokenizer.list.file(func->token); @@ -319,8 +311,16 @@ static bool isOperatorFunction(const std::string & funcName) return std::find(additionalOperators.cbegin(), additionalOperators.cend(), funcName.substr(operatorPrefix.length())) != additionalOperators.cend(); } -bool CheckUnusedFunctions::check(ErrorLogger& errorLogger, const Settings& settings) const +#define logChecker(id) \ + do { \ + const ErrorMessage errmsg({}, nullptr, Severity::internal, "logChecker", (id), CWE(0U), Certainty::normal); \ + errorLogger.reportErr(errmsg); \ + } while (false) + +bool CheckUnusedFunctions::check(const Settings& settings, ErrorLogger& errorLogger) const { + logChecker("CheckUnusedFunctions::check"); // unusedFunction + using ErrorParams = std::tuple; std::vector errors; // ensure well-defined order @@ -367,23 +367,6 @@ void CheckUnusedFunctions::unusedFunctionError(ErrorLogger& errorLogger, errorLogger.reportErr(errmsg); } -void CheckUnusedFunctions::parseTokens(const Tokenizer &tokenizer, const Settings &settings) -{ - instance.parseTokens(tokenizer, tokenizer.list.getFiles().front().c_str(), settings); -} - -#define logChecker(id) \ - do { \ - const ErrorMessage errmsg({}, nullptr, Severity::internal, "logChecker", (id), CWE(0U), Certainty::normal); \ - errorLogger.reportErr(errmsg); \ - } while (false) - -bool CheckUnusedFunctions::check(const Settings& settings, ErrorLogger &errorLogger) -{ - logChecker("CheckUnusedFunctions::check"); // unusedFunction - return instance.check(errorLogger, settings); -} - CheckUnusedFunctions::FunctionDecl::FunctionDecl(const Function *f) : functionName(f->name()), lineNumber(f->token->linenr()) {} @@ -475,3 +458,23 @@ void CheckUnusedFunctions::analyseWholeProgram(const Settings &settings, ErrorLo } } } + +void CheckUnusedFunctions::updateFunctionData(const CheckUnusedFunctions& check) +{ + for (const auto& entry : check.mFunctions) + { + FunctionUsage &usage = mFunctions[entry.first]; + if (!usage.lineNumber) + usage.lineNumber = entry.second.lineNumber; + // TODO: why always overwrite this but not the filename and line? + usage.fileIndex = entry.second.fileIndex; + if (usage.filename.empty()) + usage.filename = entry.second.filename; + // cppcheck-suppress bitwiseOnBoolean - TODO: FP + usage.usedOtherFile |= entry.second.usedOtherFile; + // cppcheck-suppress bitwiseOnBoolean - TODO: FP + usage.usedSameFile |= entry.second.usedSameFile; + } + mFunctionDecl.insert(mFunctionDecl.cend(), check.mFunctionDecl.cbegin(), check.mFunctionDecl.cend()); + mFunctionCalls.insert(check.mFunctionCalls.cbegin(), check.mFunctionCalls.cend()); +} diff --git a/lib/checkunusedfunctions.h b/lib/checkunusedfunctions.h index 6db37f95aeb..462ea734385 100644 --- a/lib/checkunusedfunctions.h +++ b/lib/checkunusedfunctions.h @@ -50,9 +50,7 @@ class CPPCHECKLIB CheckUnusedFunctions { // Parse current tokens and determine.. // * Check what functions are used // * What functions are declared - void parseTokens(const Tokenizer &tokenizer, const char FileName[], const Settings &settings); - - static void parseTokens(const Tokenizer &tokenizer, const Settings &settings); + void parseTokens(const Tokenizer &tokenizer, const Settings &settings); std::string analyzerInfo() const; @@ -62,14 +60,12 @@ class CPPCHECKLIB CheckUnusedFunctions { unusedFunctionError(errorLogger, emptyString, 0, 0, "funcName"); } - static bool check(const Settings& settings, ErrorLogger &errorLogger); - -private: - static void clear(); - // Return true if an error is reported. - bool check(ErrorLogger& errorLogger, const Settings& settings) const; + bool check(const Settings& settings, ErrorLogger& errorLogger) const; + + void updateFunctionData(const CheckUnusedFunctions& check); +private: static void unusedFunctionError(ErrorLogger& errorLogger, const std::string &filename, unsigned int fileIndex, unsigned int lineNumber, const std::string &funcname); diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index 6eacafa88b6..3a2e566dfdf 100644 --- a/lib/cppcheck.cpp +++ b/lib/cppcheck.cpp @@ -429,6 +429,9 @@ static bool reportClangErrors(std::istream &is, const std::functionupdateFunctionData(*temp.mUnusedFunctionsCheck); + return returnValue; } const unsigned int returnValue = temp.checkFile(Path::simplifyPath(fs.filename), fs.cfg); mSettings.supprs.nomsg.addSuppressions(temp.mSettings.supprs.nomsg.getSuppressions()); + if (mUnusedFunctionsCheck) + mUnusedFunctionsCheck->updateFunctionData(*temp.mUnusedFunctionsCheck); return returnValue; } @@ -595,6 +608,10 @@ static simplecpp::TokenList createTokenList(const std::string& filename, std::ve unsigned int CppCheck::checkFile(const std::string& filename, const std::string &cfgname, std::istream* fileStream) { + // TODO: move to constructor when CppCheck no longer owns the settings + if (mSettings.checks.isEnabled(Checks::unusedFunction) && !mUnusedFunctionsCheck) + mUnusedFunctionsCheck.reset(new CheckUnusedFunctions()); + mExitCode = 0; FilesDeleter filesDeleter; @@ -633,7 +650,7 @@ unsigned int CppCheck::checkFile(const std::string& filename, const std::string try { if (mSettings.library.markupFile(filename)) { - if (mSettings.isUnusedFunctionCheckEnabled() && mSettings.buildDir.empty()) { + if (mUnusedFunctionsCheck && mSettings.isUnusedFunctionCheckEnabled() && mSettings.buildDir.empty()) { Tokenizer tokenizer(mSettings, this); if (fileStream) tokenizer.list.createTokens(*fileStream, filename); @@ -641,7 +658,7 @@ unsigned int CppCheck::checkFile(const std::string& filename, const std::string std::ifstream in(filename); tokenizer.list.createTokens(in, filename); } - CheckUnusedFunctions::parseTokens(tokenizer, mSettings); + mUnusedFunctionsCheck->parseTokens(tokenizer, mSettings); } return EXIT_SUCCESS; } @@ -1103,10 +1120,10 @@ void CppCheck::checkNormalTokens(const Tokenizer &tokenizer) } if (mSettings.checks.isEnabled(Checks::unusedFunction) && !mSettings.buildDir.empty()) { - unusedFunctionsChecker.parseTokens(tokenizer, tokenizer.list.getFiles().front().c_str(), mSettings); + unusedFunctionsChecker.parseTokens(tokenizer, mSettings); } - if (mSettings.isUnusedFunctionCheckEnabled() && mSettings.buildDir.empty()) { - CheckUnusedFunctions::parseTokens(tokenizer, mSettings); + if (mUnusedFunctionsCheck && mSettings.isUnusedFunctionCheckEnabled() && mSettings.buildDir.empty()) { + mUnusedFunctionsCheck->parseTokens(tokenizer, mSettings); } if (mSettings.clang) { @@ -1789,8 +1806,8 @@ bool CppCheck::analyseWholeProgram() for (Check *check : Check::instances()) errors |= check->analyseWholeProgram(&ctu, mFileInfo, mSettings, *this); // TODO: ctu - if (mSettings.checks.isEnabled(Checks::unusedFunction)) - errors |= CheckUnusedFunctions::check(mSettings, *this); + if (mUnusedFunctionsCheck) + errors |= mUnusedFunctionsCheck->check(mSettings, *this); return errors && (mExitCode > 0); } @@ -1856,8 +1873,8 @@ void CppCheck::analyseWholeProgram(const std::string &buildDir, const std::list< for (Check *check : Check::instances()) check->analyseWholeProgram(&ctuFileInfo, fileInfoList, mSettings, *this); - if (mSettings.checks.isEnabled(Checks::unusedFunction)) - CheckUnusedFunctions::check(mSettings, *this); + if (mUnusedFunctionsCheck) + mUnusedFunctionsCheck->check(mSettings, *this); for (Check::FileInfo *fi : fileInfoList) delete fi; diff --git a/lib/cppcheck.h b/lib/cppcheck.h index d08fc525c53..f8f2627d1af 100644 --- a/lib/cppcheck.h +++ b/lib/cppcheck.h @@ -42,6 +42,7 @@ class Tokenizer; enum class SHOWTIME_MODES; struct FileSettings; +class CheckUnusedFunctions; /// @addtogroup Core /// @{ @@ -248,6 +249,8 @@ class CPPCHECKLIB CppCheck : ErrorLogger { ExecuteCmdFn mExecuteCommand; std::ofstream mPlistFile; + + std::unique_ptr mUnusedFunctionsCheck; }; /// @} diff --git a/test/cli/unusedFunction/3.c b/test/cli/unusedFunction/3.c index 876803c585b..3b517fe3c36 100644 --- a/test/cli/unusedFunction/3.c +++ b/test/cli/unusedFunction/3.c @@ -1,3 +1,3 @@ void f3_1() {} void f3_2() {} -void f3_3() {} \ No newline at end of file +void f3_3() {} \ No newline at end of file diff --git a/test/cli/unusedFunction/4.c b/test/cli/unusedFunction/4.c new file mode 100644 index 00000000000..ae17ea51075 --- /dev/null +++ b/test/cli/unusedFunction/4.c @@ -0,0 +1,7 @@ +static void f4_0() {} + +// cppcheck-suppress unusedFunction +void f4_1() +{ + f4_0(); +} \ No newline at end of file diff --git a/test/cli/unusedFunction/unusedFunction.cppcheck b/test/cli/unusedFunction/unusedFunction.cppcheck index 858bee444a6..fd28f7074a9 100644 --- a/test/cli/unusedFunction/unusedFunction.cppcheck +++ b/test/cli/unusedFunction/unusedFunction.cppcheck @@ -4,5 +4,6 @@ + \ No newline at end of file diff --git a/test/cli/unused_function_test.py b/test/cli/unused_function_test.py index b15b35540b4..e161df4e06f 100644 --- a/test/cli/unused_function_test.py +++ b/test/cli/unused_function_test.py @@ -14,23 +14,17 @@ # TODO: make this a generic helper function def __create_compdb(tmpdir, projpath): compile_commands = os.path.join(tmpdir, 'compile_commands.json') - j = [ - { + files = [] + for f in os.listdir(projpath): + files.append(f) + files.sort() + j = [] + for f in files: + j.append({ 'directory': projpath, - 'file': os.path.join(projpath, '1.c'), - 'command': 'gcc -c 1.c' - }, - { - 'directory': projpath, - 'file': os.path.join(projpath, '2.c'), - 'command': 'gcc -c 2.c' - }, - { - 'directory': projpath, - 'file': os.path.join(projpath, '3.c'), - 'command': 'gcc -c 3.c' - } - ] + 'file': os.path.join(projpath, f), + 'command': 'gcc -c {}'.format(f) + }) with open(compile_commands, 'wt') as f: f.write(json.dumps(j, indent=4)) return compile_commands @@ -132,7 +126,8 @@ def test_unused_functions_builddir_j(tmpdir): assert stderr.splitlines() == [ "{}1.c:4:0: style: The function 'f1' is never used. [unusedFunction]".format(__project_dir_sep), "{}2.c:4:0: style: The function 'f2' is never used. [unusedFunction]".format(__project_dir_sep), - "{}3.c:3:0: style: The function 'f3_3' is never used. [unusedFunction]".format(__project_dir_sep) + "{}3.c:3:0: style: The function 'f3_3' is never used. [unusedFunction]".format(__project_dir_sep), + "{}4.c:4:0: style: The function 'f4_1' is never used. [unusedFunction]".format(__project_dir_sep) ] assert ret == 0, stdout @@ -168,7 +163,8 @@ def test_unused_functions_builddir_project_j(tmpdir): assert stderr.splitlines() == [ "{}1.c:4:0: style: The function 'f1' is never used. [unusedFunction]".format(__project_dir_sep), "{}2.c:4:0: style: The function 'f2' is never used. [unusedFunction]".format(__project_dir_sep), - "{}3.c:3:0: style: The function 'f3_3' is never used. [unusedFunction]".format(__project_dir_sep) + "{}3.c:3:0: style: The function 'f3_3' is never used. [unusedFunction]".format(__project_dir_sep), + "{}4.c:4:0: style: The function 'f4_1' is never used. [unusedFunction]".format(__project_dir_sep) ] assert ret == 0, stdout @@ -208,6 +204,7 @@ def test_unused_functions_builddir_compdb_j(tmpdir): assert stderr.splitlines() == [ "{}1.c:4:0: style: The function 'f1' is never used. [unusedFunction]".format(__project_dir_sep), "{}2.c:4:0: style: The function 'f2' is never used. [unusedFunction]".format(__project_dir_sep), - "{}3.c:3:0: style: The function 'f3_3' is never used. [unusedFunction]".format(__project_dir_sep) + "{}3.c:3:0: style: The function 'f3_3' is never used. [unusedFunction]".format(__project_dir_sep), + "{}4.c:4:0: style: The function 'f4_1' is never used. [unusedFunction]".format(__project_dir_sep) ] assert ret == 0, stdout diff --git a/test/testprocessexecutor.cpp b/test/testprocessexecutor.cpp index 0ec6b90a18c..808f9ec49ff 100644 --- a/test/testprocessexecutor.cpp +++ b/test/testprocessexecutor.cpp @@ -69,8 +69,6 @@ class TestProcessExecutorBase : public TestFixture { void check(unsigned int jobs, int files, int result, const std::string &data, const CheckOptions& opt = make_default_obj{}) { errout.str(""); - CheckUnusedFunctions::clear(); - std::list fileSettings; std::list> filelist; diff --git a/test/testsingleexecutor.cpp b/test/testsingleexecutor.cpp index 780c4572100..8062fed5ea1 100644 --- a/test/testsingleexecutor.cpp +++ b/test/testsingleexecutor.cpp @@ -74,8 +74,6 @@ class TestSingleExecutorBase : public TestFixture { void check(int files, int result, const std::string &data, const CheckOptions& opt = make_default_obj{}) { errout.str(""); - CheckUnusedFunctions::clear(); - std::list fileSettings; std::list> filelist; diff --git a/test/testsuppressions.cpp b/test/testsuppressions.cpp index 674f80c8d7c..0f80af45e4e 100644 --- a/test/testsuppressions.cpp +++ b/test/testsuppressions.cpp @@ -230,8 +230,6 @@ class TestSuppressions : public TestFixture { // Clear the error log errout.str(""); - CheckUnusedFunctions::clear(); - std::list fileSettings; std::list> filelist; @@ -284,8 +282,6 @@ class TestSuppressions : public TestFixture { unsigned int _checkSuppressionThreads(const char code[], bool useFS, const std::string &suppression = emptyString) { errout.str(""); - CheckUnusedFunctions::clear(); - std::list fileSettings; std::list> filelist; @@ -334,8 +330,6 @@ class TestSuppressions : public TestFixture { unsigned int _checkSuppressionProcesses(const char code[], bool useFS, const std::string &suppression = emptyString) { errout.str(""); - CheckUnusedFunctions::clear(); - std::list fileSettings; std::list> filelist; diff --git a/test/testthreadexecutor.cpp b/test/testthreadexecutor.cpp index 4708e45179e..9bed9009bfc 100644 --- a/test/testthreadexecutor.cpp +++ b/test/testthreadexecutor.cpp @@ -69,8 +69,6 @@ class TestThreadExecutorBase : public TestFixture { void check(unsigned int jobs, int files, int result, const std::string &data, const CheckOptions& opt = make_default_obj{}) { errout.str(""); - CheckUnusedFunctions::clear(); - std::list fileSettings; std::list> filelist; diff --git a/test/testunusedfunctions.cpp b/test/testunusedfunctions.cpp index d5235a7be93..01d5483c1c3 100644 --- a/test/testunusedfunctions.cpp +++ b/test/testunusedfunctions.cpp @@ -96,9 +96,9 @@ class TestUnusedFunctions : public TestFixture { // Check for unused functions.. CheckUnusedFunctions checkUnusedFunctions; - checkUnusedFunctions.parseTokens(tokenizer, "someFile.c", settings1); + checkUnusedFunctions.parseTokens(tokenizer, settings1); // check() returns error if and only if errout is not empty. - if ((checkUnusedFunctions.check)(*this, settings1)) { + if ((checkUnusedFunctions.check)(settings1, *this)) { ASSERT(!errout.str().empty()); } else { ASSERT_EQUALS("", errout.str()); @@ -556,11 +556,11 @@ class TestUnusedFunctions : public TestFixture { std::istringstream istr(code); ASSERT(tokenizer2.tokenize(istr, fname.str().c_str())); - c.parseTokens(tokenizer2, "someFile.c", settings); + c.parseTokens(tokenizer2, settings); } // Check for unused functions.. - (c.check)(*this, settings); + (c.check)(settings, *this); ASSERT_EQUALS("[test1.cpp:1]: (style) The function 'f' is never used.\n", errout.str()); }