SingleExecutor: process markup files after code when scanning projects (#4972)

* SingleExecutor: added TODOs

* test `SingleExecutor` with files and project

* SingleExecutor: process markup files after code when scanning project

* TestSingleExecutor: generate scoped files before calling executor

* CI-unixish.yml: added `--output-on-failure` to CTest call

* helpers.cpp: improved error reporting in `~ScopedFile()`

* use unique filenames in executor tests to avoid collisions

* fixed `functionStatic` selfcheck warnings

.github/workflows/CI-unixish.yml

@@ -111,7 +111,7 @@ jobs:
       - name: Run CTest
         run: |
           pushd cmake.output
-          ctest -j$(nproc)
+          ctest --output-on-failure -j$(nproc)
 
   build_uchar:
 

cli/singleexecutor.cpp

@@ -51,6 +51,8 @@ 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 (mSettings.project.fileSettings.empty()) {
         for (std::map<std::string, std::size_t>::const_iterator i = mFiles.cbegin(); i != mFiles.cend(); ++i) {
             if (!mSettings.library.markupFile(i->first)
@@ -59,6 +61,7 @@ unsigned int SingleExecutor::check()
                 processedsize += i->second;
                 if (!mSettings.quiet)
                     reportStatus(c + 1, mFiles.size(), processedsize, totalfilesize);
+                // TODO: call analyseClangTidy()
                 c++;
             }
         }
@@ -66,6 +69,8 @@ unsigned int SingleExecutor::check()
         // filesettings
         // check all files of the project
         for (const ImportProject::FileSettings &fs : mSettings.project.fileSettings) {
+            if (!mSettings.library.markupFile(fs.filename)
+                || !mSettings.library.processMarkupAfterCode(fs.filename)) {
                 result += mCppcheck.check(fs);
                 ++c;
                 if (!mSettings.quiet)
@@ -74,18 +79,37 @@ unsigned int SingleExecutor::check()
                     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 (mSettings.project.fileSettings.empty()) {
         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)
+                && 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++;
             }
         }
+    }
+    else {
+        for (const ImportProject::FileSettings &fs : mSettings.project.fileSettings) {
+            if (mSettings.library.markupFile(fs.filename)
+                && mSettings.library.processMarkupAfterCode(fs.filename)) {
+                result += mCppcheck.check(fs);
+                ++c;
+                if (!mSettings.quiet)
+                    reportStatus(c, mSettings.project.fileSettings.size(), c, mSettings.project.fileSettings.size());
+                if (mSettings.clangTidy)
+                    mCppcheck.analyseClangTidy(fs);
+            }
+        }
+    }
     if (mCppcheck.analyseWholeProgram())
         result++;
 

lib/library.h

@@ -53,7 +53,7 @@ namespace tinyxml2 {
 class CPPCHECKLIB Library {
     // TODO: get rid of this
     friend class TestSymbolDatabase; // For testing only
-    friend class TestSingleExecutor; // For testing only
+    friend class TestSingleExecutorBase; // For testing only
     friend class TestThreadExecutor; // For testing only
     friend class TestProcessExecutor; // For testing only
 

releasenotes.txt

@@ -14,3 +14,4 @@ release notes for cppcheck-2.11
     - `constVariableReference`
     - `constVariablePointer`
 - More command-line parameters will now check if the given integer argument is actually valid. Several other internal string-to-integer conversions will not be error checked.
+- scanning projects (with -j1) will now defer the analysis of markup files until the whole code was processed

test/helpers.cpp

@@ -22,6 +22,7 @@
 #include "preprocessor.h"
 
 #include <cstdio>
+#include <iostream>
 #include <fstream>
 #include <stdexcept>
 #include <utility>
@@ -60,12 +61,21 @@ ScopedFile::ScopedFile(std::string name, const std::string &content, std::string
 }
 
 ScopedFile::~ScopedFile() {
-    std::remove(mFullPath.c_str());
+    const int remove_res = std::remove(mFullPath.c_str());
+    if (remove_res != 0) {
+        std::cout << "ScopedFile(" << mFullPath + ") - could not delete file (" << remove_res << ")";
+    }
     if (!mPath.empty() && mPath != Path::getCurrentPath()) {
 #ifdef _WIN32
-        RemoveDirectoryA(mPath.c_str());
+        if (!RemoveDirectoryA(mPath.c_str())) {
+            std::cout << "ScopedFile(" << mFullPath + ") - could not delete folder (" << GetLastError() << ")";
+        }
 #else
-        rmdir(mPath.c_str());
+        const int rmdir_res = rmdir(mPath.c_str());
+        if (rmdir_res == -1) {
+            const int err = errno;
+            std::cout << "ScopedFile(" << mFullPath + ") - could not delete folder (" << err << ")";
+        }
 #endif
     }
 }

test/testprocessexecutor.cpp

@@ -41,6 +41,11 @@ public:
 private:
     Settings settings = settingsBuilder().library("std.cfg").build();
 
+    static std::string fprefix()
+    {
+        return "process";
+    }
+
     /**
      * Execute check using n jobs for y files which are have
      * identical data, given within data.
@@ -53,7 +58,7 @@ private:
         if (filesList.empty()) {
             for (int i = 1; i <= files; ++i) {
                 std::ostringstream oss;
-                oss << "file_" << i << ".cpp";
+                oss << fprefix() << "_" << i << ".cpp";
                 filemap[oss.str()] = data.size();
             }
         }
@@ -186,7 +191,7 @@ private:
         settings.library.mProcessAfterCode.emplace(".cp1", true);
 
         const std::vector<std::string> files = {
-            "file_1.cp1", "file_2.cpp", "file_3.cp1", "file_4.cpp"
+            fprefix() + "_1.cp1", fprefix() + "_2.cpp", fprefix() + "_3.cp1", fprefix() + "_4.cpp"
         };
 
         // the checks are not executed on the markup files => expected result is 2
@@ -198,21 +203,21 @@ private:
               "}",
               SHOWTIME_MODES::SHOWTIME_NONE, nullptr, files);
         // TODO: order of "Checking" and "checked" is affected by thread
-        /*TODO_ASSERT_EQUALS("Checking file_2.cpp ...\n"
+        /*TODO_ASSERT_EQUALS("Checking " + fprefix() + "_2.cpp ...\n"
                            "1/4 files checked 25% done\n"
-                           "Checking file_4.cpp ...\n"
+                           "Checking " + fprefix() + "_4.cpp ...\n"
                            "2/4 files checked 50% done\n"
-                           "Checking file_1.cp1 ...\n"
+                           "Checking " + fprefix() + "_1.cp1 ...\n"
                            "3/4 files checked 75% done\n"
-                           "Checking file_3.cp1 ...\n"
+                           "Checking " + fprefix() + "_3.cp1 ...\n"
                            "4/4 files checked 100% done\n",
-                           "Checking file_1.cp1 ...\n"
+                           "Checking " + fprefix() + "_1.cp1 ...\n"
                            "1/4 files checked 25% done\n"
-                           "Checking file_2.cpp ...\n"
+                           "Checking " + fprefix() + "_2.cpp ...\n"
                            "2/4 files checked 50% done\n"
-                           "Checking file_3.cp1 ...\n"
+                           "Checking " + fprefix() + "_3.cp1 ...\n"
                            "3/4 files checked 75% done\n"
-                           "Checking file_4.cpp ...\n"
+                           "Checking " + fprefix() + "_4.cpp ...\n"
                            "4/4 files checked 100% done\n",
                            output.str());*/
         settings = settingsOld;

test/testsingleexecutor.cpp

@@ -36,12 +36,20 @@
 #include <utility>
 #include <vector>
 
-class TestSingleExecutor : public TestFixture {
+class TestSingleExecutorBase : public TestFixture {
-public:
+protected:
-    TestSingleExecutor() : TestFixture("TestSingleExecutor") {}
+    TestSingleExecutorBase(const char * const name, bool useFS) : TestFixture(name), useFS(useFS) {}
 
 private:
     Settings settings = settingsBuilder().library("std.cfg").build();
+    bool useFS;
+
+    std::string fprefix() const
+    {
+        if (useFS)
+            return "singlefs";
+        return "single";
+    }
 
     static std::string zpad3(int i)
     {
@@ -55,18 +63,29 @@ private:
     void check(int files, int result, const std::string &data, SHOWTIME_MODES showtime = SHOWTIME_MODES::SHOWTIME_NONE, const char* const plistOutput = nullptr, const std::vector<std::string>& filesList = {}) {
         errout.str("");
         output.str("");
+        settings.project.fileSettings.clear();
 
         std::map<std::string, std::size_t> filemap;
         if (filesList.empty()) {
             for (int i = 1; i <= files; ++i) {
-                const std::string s = "file_" + zpad3(i) + ".cpp";
+                const std::string s = fprefix() + "_" + zpad3(i) + ".cpp";
                 filemap[s] = data.size();
+                if (useFS) {
+                    ImportProject::FileSettings fs;
+                    fs.filename = s;
+                    settings.project.fileSettings.emplace_back(std::move(fs));
+                }
             }
         }
         else {
             for (const auto& f : filesList)
             {
                 filemap[f] = data.size();
+                if (useFS) {
+                    ImportProject::FileSettings fs;
+                    fs.filename = f;
+                    settings.project.fileSettings.emplace_back(std::move(fs));
+                }
             }
         }
 
@@ -78,13 +97,18 @@ private:
             return false;
         });
         cppcheck.settings() = settings;
-        // TODO: test with settings.project.fileSettings;
+
-        SingleExecutor executor(cppcheck, filemap, settings, *this);
         std::vector<std::unique_ptr<ScopedFile>> scopedfiles;
         scopedfiles.reserve(filemap.size());
         for (std::map<std::string, std::size_t>::const_iterator i = filemap.cbegin(); i != filemap.cend(); ++i)
             scopedfiles.emplace_back(new ScopedFile(i->first, data));
 
+        // clear files list so only fileSettings are used
+        if (useFS)
+            filemap.clear();
+
+        // TODO: test with settings.project.fileSettings;
+        SingleExecutor executor(cppcheck, filemap, settings, *this);
         ASSERT_EQUALS(result, executor.check());
     }
 
@@ -112,7 +136,7 @@ private:
               "}");
         std::string expected;
         for (int i = 1; i <= 100; ++i) {
-            expected += "Checking file_" + zpad3(i) + ".cpp ...\n";
+            expected += "Checking " + fprefix() + "_" + zpad3(i) + ".cpp ...\n";
             expected += std::to_string(i) + "/100 files checked " + std::to_string(i) + "% done\n";
         }
         ASSERT_EQUALS(expected, output.str());
@@ -190,7 +214,7 @@ private:
         settings.library.mProcessAfterCode.emplace(".cp1", true);
 
         const std::vector<std::string> files = {
-            "file_1.cp1", "file_2.cpp", "file_3.cp1", "file_4.cpp"
+            fprefix() + "_1.cp1", fprefix() + "_2.cpp", fprefix() + "_3.cp1", fprefix() + "_4.cpp"
         };
 
         // checks are not executed on markup files => expected result is 2
@@ -202,13 +226,13 @@ private:
               "}",
               SHOWTIME_MODES::SHOWTIME_NONE, nullptr, files);
         // TODO: filter out the "files checked" messages
-        ASSERT_EQUALS("Checking file_2.cpp ...\n"
+        ASSERT_EQUALS("Checking " + fprefix() + "_2.cpp ...\n"
                       "1/4 files checked 25% done\n"
-                      "Checking file_4.cpp ...\n"
+                      "Checking " + fprefix() + "_4.cpp ...\n"
                       "2/4 files checked 50% done\n"
-                      "Checking file_1.cp1 ...\n"
+                      "Checking " + fprefix() + "_1.cp1 ...\n"
                       "3/4 files checked 75% done\n"
-                      "Checking file_3.cp1 ...\n"
+                      "Checking " + fprefix() + "_3.cp1 ...\n"
                       "4/4 files checked 100% done\n", output.str());
         settings = settingsOld;
     }
@@ -217,4 +241,15 @@ private:
     // TODO: test whole program analysis
 };
 
-REGISTER_TEST(TestSingleExecutor)
+class TestSingleExecutorFiles : public TestSingleExecutorBase {
+public:
+    TestSingleExecutorFiles() : TestSingleExecutorBase("TestSingleExecutorFiles", false) {}
+};
+
+class TestSingleExecutorFS : public TestSingleExecutorBase {
+public:
+    TestSingleExecutorFS() : TestSingleExecutorBase("TestSingleExecutorFS", true) {}
+};
+
+REGISTER_TEST(TestSingleExecutorFiles)
+REGISTER_TEST(TestSingleExecutorFS)

test/testthreadexecutor.cpp

@@ -41,6 +41,11 @@ public:
 private:
     Settings settings = settingsBuilder().library("std.cfg").build();
 
+    static std::string fprefix()
+    {
+        return "thread";
+    }
+
     /**
      * Execute check using n jobs for y files which are have
      * identical data, given within data.
@@ -53,7 +58,7 @@ private:
         if (filesList.empty()) {
             for (int i = 1; i <= files; ++i) {
                 std::ostringstream oss;
-                oss << "file_" << i << ".cpp";
+                oss << fprefix() << "_" << i << ".cpp";
                 filemap[oss.str()] = data.size();
             }
         }
@@ -184,7 +189,7 @@ private:
         settings.library.mProcessAfterCode.emplace(".cp1", true);
 
         const std::vector<std::string> files = {
-            "file_1.cp1", "file_2.cpp", "file_3.cp1", "file_4.cpp"
+            fprefix() + "_1.cp1", fprefix() + "_2.cpp", fprefix() + "_3.cp1", fprefix() + "_4.cpp"
         };
 
         // checks are not executed on markup files => expected result is 2
@@ -196,21 +201,21 @@ private:
               "}",
               SHOWTIME_MODES::SHOWTIME_NONE, nullptr, files);
         // TODO: order of "Checking" and "checked" is affected by thread
-        /*TODO_ASSERT_EQUALS("Checking file_2.cpp ...\n"
+        /*TODO_ASSERT_EQUALS("Checking " + fprefix() + "_2.cpp ...\n"
                            "1/4 files checked 25% done\n"
-                           "Checking file_4.cpp ...\n"
+                           "Checking " + fprefix() + "_4.cpp ...\n"
                            "2/4 files checked 50% done\n"
-                           "Checking file_1.cp1 ...\n"
+                           "Checking " + fprefix() + "_1.cp1 ...\n"
                            "3/4 files checked 75% done\n"
-                           "Checking file_3.cp1 ...\n"
+                           "Checking " + fprefix() + "_3.cp1 ...\n"
                            "4/4 files checked 100% done\n",
-                           "Checking file_1.cp1 ...\n"
+                           "Checking " + fprefix() + "_1.cp1 ...\n"
                            "1/4 files checked 25% done\n"
-                           "Checking file_2.cpp ...\n"
+                           "Checking " + fprefix() + "_2.cpp ...\n"
                            "2/4 files checked 50% done\n"
-                           "Checking file_3.cp1 ...\n"
+                           "Checking " + fprefix() + "_3.cp1 ...\n"
                            "3/4 files checked 75% done\n"
-                           "Checking file_4.cpp ...\n"
+                           "Checking " + fprefix() + "_4.cpp ...\n"
                            "4/4 files checked 100% done\n",
                            output.str());*/
         settings = settingsOld;