From 704b862a2dab89d60ca44a75f1ebf10abc91cc98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliver=20St=C3=B6neberg?= Date: Mon, 6 Nov 2023 19:06:22 +0100 Subject: [PATCH] cleaned up the file filtering code and improved testing of it (#5622) --- cli/cppcheckexecutor.cpp | 52 ++++++++++-------- cli/executor.cpp | 6 ++- cli/singleexecutor.cpp | 87 ++++++++++++++---------------- test/cli/test-more-projects.py | 98 +++++++++++++++++++++++++++++++++- test/cli/test-other.py | 56 ++++++++++++++++++- test/cli/testutils.py | 16 +++++- 6 files changed, 240 insertions(+), 75 deletions(-) diff --git a/cli/cppcheckexecutor.cpp b/cli/cppcheckexecutor.cpp index ffe405f6a..6495779fe 100644 --- a/cli/cppcheckexecutor.cpp +++ b/cli/cppcheckexecutor.cpp @@ -189,12 +189,9 @@ bool CppCheckExecutor::parseFromArgs(Settings &settings, int argc, const char* c const std::vector& pathnames = parser.getPathNames(); const std::list& fileSettings = parser.getFileSettings(); -#if defined(_WIN32) - // For Windows we want case-insensitive path matching - const bool caseSensitive = false; -#else - const bool caseSensitive = true; -#endif + // the inputs can only be used exclusively - CmdLineParser should already handle this + assert(!(!pathnames.empty() && !fileSettings.empty())); + if (!fileSettings.empty()) { if (!settings.fileFilters.empty()) { // filter only for the selected filenames from all project files @@ -209,37 +206,48 @@ bool CppCheckExecutor::parseFromArgs(Settings &settings, int argc, const char* c else { mFileSettings = fileSettings; } - } else if (!pathnames.empty()) { + } + + if (!pathnames.empty()) { + // TODO: this should be a vector or list so the order is kept + std::map files; + // TODO: this needs to be inlined into PathMatch as it depends on the underlying filesystem +#if defined(_WIN32) + // For Windows we want case-insensitive path matching + const bool caseSensitive = false; +#else + const bool caseSensitive = true; +#endif // Execute recursiveAddFiles() to each given file parameter const PathMatch matcher(ignored, caseSensitive); for (const std::string &pathname : pathnames) { - std::string err = FileLister::recursiveAddFiles(mFiles, Path::toNativeSeparators(pathname), settings.library.markupExtensions(), matcher); + const std::string err = FileLister::recursiveAddFiles(files, Path::toNativeSeparators(pathname), settings.library.markupExtensions(), matcher); if (!err.empty()) { // TODO: bail out? logger.printMessage(err); } } + + if (!settings.fileFilters.empty()) { + std::copy_if(files.cbegin(), files.cend(), std::inserter(mFiles, mFiles.end()), [&](const decltype(files)::value_type& entry) { + return matchglobs(settings.fileFilters, entry.first); + }); + if (mFiles.empty()) { + logger.printError("could not find any files matching the filter."); + return false; + } + } + else { + mFiles = files; + } } - if (mFiles.empty() && fileSettings.empty()) { + if (mFiles.empty() && mFileSettings.empty()) { logger.printError("could not find or open any of the paths given."); if (!ignored.empty()) logger.printMessage("Maybe all paths were ignored?"); return false; } - if (!settings.fileFilters.empty() && fileSettings.empty()) { - std::map newMap; - for (std::map::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) - if (matchglobs(settings.fileFilters, i->first)) { - newMap[i->first] = i->second; - } - mFiles = newMap; - if (mFiles.empty()) { - logger.printError("could not find any files matching the filter."); - return false; - } - - } return true; } diff --git a/cli/executor.cpp b/cli/executor.cpp index 338ea79be..7febba3a7 100644 --- a/cli/executor.cpp +++ b/cli/executor.cpp @@ -24,6 +24,7 @@ #include "suppressions.h" #include +#include #include // IWYU pragma: keep #include @@ -31,7 +32,10 @@ struct FileSettings; Executor::Executor(const std::map &files, const std::list& fileSettings, const Settings &settings, Suppressions &suppressions, ErrorLogger &errorLogger) : mFiles(files), mFileSettings(fileSettings), mSettings(settings), mSuppressions(suppressions), mErrorLogger(errorLogger) -{} +{ + // the two inputs may only be used exclusively + assert(!(!files.empty() && !fileSettings.empty())); +} bool Executor::hasToLog(const ErrorMessage &msg) { diff --git a/cli/singleexecutor.cpp b/cli/singleexecutor.cpp index 2b37c956e..5a8fb2281 100644 --- a/cli/singleexecutor.cpp +++ b/cli/singleexecutor.cpp @@ -49,65 +49,56 @@ unsigned int SingleExecutor::check() std::size_t processedsize = 0; unsigned int c = 0; - // TODO: processes either mSettings.project.fileSettings or mFiles - process/thread implementations process both - // TODO: thread/process implementations process fileSettings first - if (mFileSettings.empty()) { - for (std::map::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) { - if (!mSettings.library.markupFile(i->first) - || !mSettings.library.processMarkupAfterCode(i->first)) { - result += mCppcheck.check(i->first); - processedsize += i->second; - if (!mSettings.quiet) - reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize); - // TODO: call analyseClangTidy()? - c++; - } + + for (std::map::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) { + if (!mSettings.library.markupFile(i->first) || !mSettings.library.processMarkupAfterCode(i->first)) { + result += mCppcheck.check(i->first); + processedsize += i->second; + ++c; + if (!mSettings.quiet) + reportStatus(c, mFiles.size(), processedsize, totalfilesize); + // TODO: call analyseClangTidy()? } - } else { - // filesettings - // check all files of the project - for (const FileSettings &fs : mFileSettings) { - if (!mSettings.library.markupFile(fs.filename) - || !mSettings.library.processMarkupAfterCode(fs.filename)) { - result += mCppcheck.check(fs); - ++c; - if (!mSettings.quiet) - reportStatus(c, mFileSettings.size(), c, mFileSettings.size()); - if (mSettings.clangTidy) - mCppcheck.analyseClangTidy(fs); - } + } + + // filesettings + // check all files of the project + for (const FileSettings &fs : mFileSettings) { + if (!mSettings.library.markupFile(fs.filename) || !mSettings.library.processMarkupAfterCode(fs.filename)) { + result += mCppcheck.check(fs); + ++c; + if (!mSettings.quiet) + reportStatus(c, mFileSettings.size(), c, mFileSettings.size()); + if (mSettings.clangTidy) + mCppcheck.analyseClangTidy(fs); } } // second loop to parse all markup files which may not work until all // c/cpp files have been parsed and checked // TODO: get rid of duplicated code - if (mFileSettings.empty()) { - for (std::map::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) { - if (mSettings.library.markupFile(i->first) - && mSettings.library.processMarkupAfterCode(i->first)) { - result += mCppcheck.check(i->first); - processedsize += i->second; - if (!mSettings.quiet) - reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize); - // TODO: call analyseClangTidy()? - c++; - } + for (std::map::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) { + if (mSettings.library.markupFile(i->first) && mSettings.library.processMarkupAfterCode(i->first)) { + result += mCppcheck.check(i->first); + processedsize += i->second; + ++c; + if (!mSettings.quiet) + reportStatus(c, mFiles.size(), processedsize, totalfilesize); + // TODO: call analyseClangTidy()? } } - else { - for (const FileSettings &fs : mFileSettings) { - if (mSettings.library.markupFile(fs.filename) - && mSettings.library.processMarkupAfterCode(fs.filename)) { - result += mCppcheck.check(fs); - ++c; - if (!mSettings.quiet) - reportStatus(c, mFileSettings.size(), c, mFileSettings.size()); - if (mSettings.clangTidy) - mCppcheck.analyseClangTidy(fs); - } + + for (const FileSettings &fs : mFileSettings) { + if (mSettings.library.markupFile(fs.filename) && mSettings.library.processMarkupAfterCode(fs.filename)) { + result += mCppcheck.check(fs); + ++c; + if (!mSettings.quiet) + reportStatus(c, mFileSettings.size(), c, mFileSettings.size()); + if (mSettings.clangTidy) + mCppcheck.analyseClangTidy(fs); } } + if (mCppcheck.analyseWholeProgram()) result++; diff --git a/test/cli/test-more-projects.py b/test/cli/test-more-projects.py index 0b1a4b1db..39d80b9e8 100644 --- a/test/cli/test-more-projects.py +++ b/test/cli/test-more-projects.py @@ -2,7 +2,7 @@ import json import os import pytest -from testutils import cppcheck +from testutils import cppcheck, assert_cppcheck def test_project_force_U(tmpdir): @@ -291,3 +291,99 @@ def test_clang_tidy(tmpdir): 'Checking {} ...'.format(test_file) ] assert stderr == '' + + +def test_project_file_filter(tmpdir): + test_file = os.path.join(tmpdir, 'test.cpp') + with open(test_file, 'wt') as f: + pass + + project_file = os.path.join(tmpdir, 'test.cppcheck') + with open(project_file, 'wt') as f: + f.write( + """ + + + + +""".format(test_file)) + + args = ['--file-filter=*.cpp', '--project={}'.format(project_file)] + out_lines = [ + 'Checking {} ...'.format(test_file) + ] + + assert_cppcheck(args, ec_exp=0, err_exp=[], out_exp=out_lines) + + +def test_project_file_filter_2(tmpdir): + test_file_1 = os.path.join(tmpdir, 'test.cpp') + with open(test_file_1, 'wt') as f: + pass + test_file_2 = os.path.join(tmpdir, 'test.c') + with open(test_file_2, 'wt') as f: + pass + + project_file = os.path.join(tmpdir, 'test.cppcheck') + with open(project_file, 'wt') as f: + f.write( + """ + + + + + +""".format(test_file_1, test_file_2)) + + args = ['--file-filter=*.cpp', '--project={}'.format(project_file)] + out_lines = [ + 'Checking {} ...'.format(test_file_1) + ] + + assert_cppcheck(args, ec_exp=0, err_exp=[], out_exp=out_lines) + + +def test_project_file_filter_3(tmpdir): + test_file_1 = os.path.join(tmpdir, 'test.cpp') + with open(test_file_1, 'wt') as f: + pass + test_file_2 = os.path.join(tmpdir, 'test.c') + with open(test_file_2, 'wt') as f: + pass + + project_file = os.path.join(tmpdir, 'test.cppcheck') + with open(project_file, 'wt') as f: + f.write( + """ + + + + + +""".format(test_file_1, test_file_2)) + + args = ['--file-filter=*.c', '--project={}'.format(project_file)] + out_lines = [ + 'Checking {} ...'.format(test_file_2) + ] + + assert_cppcheck(args, ec_exp=0, err_exp=[], out_exp=out_lines) + + +def test_project_file_filter_no_match(tmpdir): + project_file = os.path.join(tmpdir, 'test.cppcheck') + with open(project_file, 'wt') as f: + f.write( + """ + + + + +""") + + args = ['--file-filter=*.c', '--project={}'.format(project_file)] + out_lines = [ + 'cppcheck: error: could not find any files matching the filter.' + ] + + assert_cppcheck(args, ec_exp=1, err_exp=[], out_exp=out_lines) diff --git a/test/cli/test-other.py b/test/cli/test-other.py index dffa3297f..89b6e4467 100644 --- a/test/cli/test-other.py +++ b/test/cli/test-other.py @@ -4,7 +4,7 @@ import os import pytest -from testutils import cppcheck +from testutils import cppcheck, assert_cppcheck def __test_missing_include(tmpdir, use_j): @@ -578,3 +578,57 @@ def test_missing_addon(tmpdir): 'Did not find addon misra3.py' ] assert stderr == "" + + +def test_file_filter(tmpdir): + test_file = os.path.join(tmpdir, 'test.cpp') + with open(test_file, 'wt') as f: + pass + + args = ['--file-filter=*.cpp', test_file] + out_lines = [ + 'Checking {} ...'.format(test_file) + ] + + assert_cppcheck(args, ec_exp=0, err_exp=[], out_exp=out_lines) + + +def test_file_filter_2(tmpdir): + test_file_1 = os.path.join(tmpdir, 'test.cpp') + with open(test_file_1, 'wt') as f: + pass + test_file_2 = os.path.join(tmpdir, 'test.c') + with open(test_file_2, 'wt') as f: + pass + + args = ['--file-filter=*.cpp', test_file_1, test_file_2] + out_lines = [ + 'Checking {} ...'.format(test_file_1) + ] + + assert_cppcheck(args, ec_exp=0, err_exp=[], out_exp=out_lines) + + +def test_file_filter_3(tmpdir): + test_file_1 = os.path.join(tmpdir, 'test.cpp') + with open(test_file_1, 'wt') as f: + pass + test_file_2 = os.path.join(tmpdir, 'test.c') + with open(test_file_2, 'wt') as f: + pass + + args = ['--file-filter=*.c', test_file_1, test_file_2] + out_lines = [ + 'Checking {} ...'.format(test_file_2) + ] + + assert_cppcheck(args, ec_exp=0, err_exp=[], out_exp=out_lines) + + +def test_file_filter_no_match(tmpdir): + args = ['--file-filter=*.c', 'test.cpp'] + out_lines = [ + 'cppcheck: error: could not find any files matching the filter.' + ] + + assert_cppcheck(args, ec_exp=1, err_exp=[], out_exp=out_lines) diff --git a/test/cli/testutils.py b/test/cli/testutils.py index 1e53c65db..61f0864d5 100644 --- a/test/cli/testutils.py +++ b/test/cli/testutils.py @@ -43,7 +43,7 @@ def create_gui_project_file(project_file, root_path=None, import_project=None, p f.close() -def lookup_cppcheck_exe(): +def __lookup_cppcheck_exe(): # path the script is located in script_path = os.path.dirname(os.path.realpath(__file__)) @@ -64,7 +64,7 @@ def lookup_cppcheck_exe(): # Run Cppcheck with args def cppcheck(args, env=None): - exe = lookup_cppcheck_exe() + exe = __lookup_cppcheck_exe() assert exe is not None, 'no cppcheck binary found' logging.info(exe + ' ' + ' '.join(args)) @@ -75,3 +75,15 @@ def cppcheck(args, env=None): if stdout.find('\nActive checkers:') > 0: stdout = stdout[:1 + stdout.find('\nActive checkers:')] return p.returncode, stdout, stderr + + +def assert_cppcheck(args, ec_exp=None, out_exp=None, err_exp=None, env=None): + exitcode, stdout, stderr = cppcheck(args, env) + if ec_exp is not None: + assert exitcode == ec_exp, stdout + if out_exp is not None: + out_lines = stdout.splitlines() + assert out_lines == out_exp + if err_exp is not None: + err_lines = stderr.splitlines() + assert err_lines == err_exp