cleaned up the file filtering code and improved testing of it (#5622)

cli/cppcheckexecutor.cpp

@@ -189,12 +189,9 @@ bool CppCheckExecutor::parseFromArgs(Settings &settings, int argc, const char* c
     const std::vector<std::string>& pathnames = parser.getPathNames();
     const std::list<FileSettings>& fileSettings = parser.getFileSettings();
 
-#if defined(_WIN32)
+    // the inputs can only be used exclusively - CmdLineParser should already handle this
-    // For Windows we want case-insensitive path matching
+    assert(!(!pathnames.empty() && !fileSettings.empty()));
-    const bool caseSensitive = false;
+
-#else
-    const bool caseSensitive = true;
-#endif
     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<std::string, std::size_t> 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<std::string, std::size_t> newMap;
-        for (std::map<std::string, std::size_t>::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;
 }

cli/executor.cpp

@@ -24,6 +24,7 @@
 #include "suppressions.h"
 
 #include <algorithm>
+#include <cassert>
 #include <sstream> // IWYU pragma: keep
 #include <utility>
 
@@ -31,7 +32,10 @@ struct FileSettings;
 
 Executor::Executor(const std::map<std::string, std::size_t> &files, const std::list<FileSettings>& 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)
 {

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
+    for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
-    if (mFileSettings.empty()) {
+        if (!mSettings.library.markupFile(i->first) || !mSettings.library.processMarkupAfterCode(i->first)) {
-        for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
+            result += mCppcheck.check(i->first);
-            if (!mSettings.library.markupFile(i->first)
+            processedsize += i->second;
-                || !mSettings.library.processMarkupAfterCode(i->first)) {
+            ++c;
-                result += mCppcheck.check(i->first);
+            if (!mSettings.quiet)
-                processedsize += i->second;
+                reportStatus(c, mFiles.size(), processedsize, totalfilesize);
-                if (!mSettings.quiet)
+            // TODO: call analyseClangTidy()?
-                    reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize);
-                // TODO: call analyseClangTidy()?
-                c++;
-            }
         }
-    } else {
+    }
-        // filesettings
+
-        // check all files of the project
+    // filesettings
-        for (const FileSettings &fs : mFileSettings) {
+    // check all files of the project
-            if (!mSettings.library.markupFile(fs.filename)
+    for (const FileSettings &fs : mFileSettings) {
-                || !mSettings.library.processMarkupAfterCode(fs.filename)) {
+        if (!mSettings.library.markupFile(fs.filename) || !mSettings.library.processMarkupAfterCode(fs.filename)) {
-                result += mCppcheck.check(fs);
+            result += mCppcheck.check(fs);
-                ++c;
+            ++c;
-                if (!mSettings.quiet)
+            if (!mSettings.quiet)
-                    reportStatus(c, mFileSettings.size(), c, mFileSettings.size());
+                reportStatus(c, mFileSettings.size(), c, mFileSettings.size());
-                if (mSettings.clangTidy)
+            if (mSettings.clangTidy)
-                    mCppcheck.analyseClangTidy(fs);
+                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<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
-        for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
+        if (mSettings.library.markupFile(i->first) && mSettings.library.processMarkupAfterCode(i->first)) {
-            if (mSettings.library.markupFile(i->first)
+            result += mCppcheck.check(i->first);
-                && mSettings.library.processMarkupAfterCode(i->first)) {
+            processedsize += i->second;
-                result += mCppcheck.check(i->first);
+            ++c;
-                processedsize += i->second;
+            if (!mSettings.quiet)
-                if (!mSettings.quiet)
+                reportStatus(c, mFiles.size(), processedsize, totalfilesize);
-                    reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize);
+            // TODO: call analyseClangTidy()?
-                // TODO: call analyseClangTidy()?
-                c++;
-            }
         }
     }
-    else {
+
-        for (const FileSettings &fs : mFileSettings) {
+    for (const FileSettings &fs : mFileSettings) {
-            if (mSettings.library.markupFile(fs.filename)
+        if (mSettings.library.markupFile(fs.filename) && mSettings.library.processMarkupAfterCode(fs.filename)) {
-                && mSettings.library.processMarkupAfterCode(fs.filename)) {
+            result += mCppcheck.check(fs);
-                result += mCppcheck.check(fs);
+            ++c;
-                ++c;
+            if (!mSettings.quiet)
-                if (!mSettings.quiet)
+                reportStatus(c, mFileSettings.size(), c, mFileSettings.size());
-                    reportStatus(c, mFileSettings.size(), c, mFileSettings.size());
+            if (mSettings.clangTidy)
-                if (mSettings.clangTidy)
+                mCppcheck.analyseClangTidy(fs);
-                    mCppcheck.analyseClangTidy(fs);
-            }
         }
     }
+
     if (mCppcheck.analyseWholeProgram())
         result++;
 

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(
+            """<?xml version="1.0" encoding="UTF-8"?>
+<project>
+    <paths>
+        <dir name="{}"/>
+    </paths>
+</project>""".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(
+            """<?xml version="1.0" encoding="UTF-8"?>
+<project>
+    <paths>
+        <dir name="{}"/>
+        <dir name="{}"/>
+    </paths>
+</project>""".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(
+            """<?xml version="1.0" encoding="UTF-8"?>
+<project>
+    <paths>
+        <dir name="{}"/>
+        <dir name="{}"/>
+    </paths>
+</project>""".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(
+            """<?xml version="1.0" encoding="UTF-8"?>
+<project>
+    <paths>
+        <dir name="test.cpp"/>
+    </paths>
+</project>""")
+
+    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)

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)

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