de-coupled ErrorLogger interface from ThreadExecutor (#3849)

cli/threadexecutor.cpp

@@ -22,6 +22,7 @@
 #include "config.h"
 #include "cppcheck.h"
 #include "cppcheckexecutor.h"
+#include "errorlogger.h"
 #include "errortypes.h"
 #include "importproject.h"
 #include "settings.h"
@@ -48,6 +49,9 @@
 #include <fcntl.h>
 #include <csignal>
 #include <unistd.h>
+
+// required for FD_ZERO
+using std::memset;
 #endif
 
 #ifdef THREADING_MODEL_WIN
@@ -55,21 +59,9 @@
 #include <numeric>
 #endif
 
-// required for FD_ZERO
-using std::memset;
-
 ThreadExecutor::ThreadExecutor(const std::map<std::string, std::size_t> &files, Settings &settings, ErrorLogger &errorLogger)
     : mFiles(files), mSettings(settings), mErrorLogger(errorLogger), mFileCount(0)
-{
+{}
-#if defined(THREADING_MODEL_FORK)
-    mWpipe = 0;
-#elif defined(THREADING_MODEL_WIN)
-    mProcessedFiles = 0;
-    mTotalFiles = 0;
-    mProcessedSize = 0;
-    mTotalFileSize = 0;
-#endif
-}
 
 ThreadExecutor::~ThreadExecutor()
 {}
@@ -80,22 +72,75 @@ void ThreadExecutor::addFileContent(const std::string &path, const std::string &
     mFileContents[path] = content;
 }
 
-void ThreadExecutor::reportErr(const ErrorMessage &msg)
-{
-    report(msg, MessageType::REPORT_ERROR);
-}
-
-void ThreadExecutor::reportInfo(const ErrorMessage &msg)
-{
-    report(msg, MessageType::REPORT_INFO);
-}
-
 ///////////////////////////////////////////////////////////////////////////////
 ////// This code is for platforms that support fork() only ////////////////////
 ///////////////////////////////////////////////////////////////////////////////
 
 #if defined(THREADING_MODEL_FORK)
 
+class PipeWriter : public ErrorLogger {
+public:
+    enum PipeSignal {REPORT_OUT='1',REPORT_ERROR='2', REPORT_INFO='3', REPORT_VERIFICATION='4', CHILD_END='5'};
+
+    explicit PipeWriter(int pipe) : mWpipe(pipe) {}
+
+    void reportOut(const std::string &outmsg, Color c) override {
+        writeToPipe(REPORT_OUT, ::toString(c) + outmsg + ::toString(Color::Reset));
+    }
+
+    void reportErr(const ErrorMessage &msg) override {
+        report(msg, MessageType::REPORT_ERROR);
+    }
+
+    void reportInfo(const ErrorMessage &msg) override {
+        report(msg, MessageType::REPORT_INFO);
+    }
+
+    void bughuntingReport(const std::string &str) override {
+        writeToPipe(REPORT_VERIFICATION, str);
+    }
+
+    void writeEnd(const std::string& str) {
+        writeToPipe(CHILD_END, str);
+    }
+
+private:
+    enum class MessageType {REPORT_ERROR, REPORT_INFO};
+
+    void report(const ErrorMessage &msg, MessageType msgType) {
+        PipeSignal pipeSignal;
+        switch (msgType) {
+        case MessageType::REPORT_ERROR:
+            pipeSignal = REPORT_ERROR;
+            break;
+        case MessageType::REPORT_INFO:
+            pipeSignal = REPORT_INFO;
+            break;
+        }
+
+        writeToPipe(pipeSignal, msg.serialize());
+    }
+
+    void writeToPipe(PipeSignal type, const std::string &data)
+    {
+        unsigned int len = static_cast<unsigned int>(data.length() + 1);
+        char *out = new char[len + 1 + sizeof(len)];
+        out[0] = static_cast<char>(type);
+        std::memcpy(&(out[1]), &len, sizeof(len));
+        std::memcpy(&(out[1+sizeof(len)]), data.c_str(), len);
+        if (write(mWpipe, out, len + 1 + sizeof(len)) <= 0) {
+            delete[] out;
+            out = nullptr;
+            std::cerr << "#### ThreadExecutor::writeToPipe, Failed to write to pipe" << std::endl;
+            std::exit(EXIT_FAILURE);
+        }
+
+        delete[] out;
+    }
+
+    const int mWpipe;
+};
+
 int ThreadExecutor::handleRead(int rpipe, unsigned int &result)
 {
     char type = 0;
@@ -108,7 +153,7 @@ int ThreadExecutor::handleRead(int rpipe, unsigned int &result)
         return -1;
     }
 
-    if (type != REPORT_OUT && type != REPORT_ERROR && type != REPORT_INFO && type != CHILD_END) {
+    if (type != PipeWriter::REPORT_OUT && type != PipeWriter::REPORT_ERROR && type != PipeWriter::REPORT_INFO && type != PipeWriter::CHILD_END) {
         std::cerr << "#### ThreadExecutor::handleRead error, type was:" << type << std::endl;
         std::exit(EXIT_FAILURE);
     }
@@ -129,9 +174,9 @@ int ThreadExecutor::handleRead(int rpipe, unsigned int &result)
     }
     buf[readIntoBuf] = 0;
 
-    if (type == REPORT_OUT) {
+    if (type == PipeWriter::REPORT_OUT) {
         mErrorLogger.reportOut(buf);
-    } else if (type == REPORT_ERROR || type == REPORT_INFO) {
+    } else if (type == PipeWriter::REPORT_ERROR || type == PipeWriter::REPORT_INFO) {
         ErrorMessage msg;
         try {
             msg.deserialize(buf);
@@ -145,13 +190,13 @@ int ThreadExecutor::handleRead(int rpipe, unsigned int &result)
             std::string errmsg = msg.toString(mSettings.verbose);
             if (std::find(mErrorList.begin(), mErrorList.end(), errmsg) == mErrorList.end()) {
                 mErrorList.emplace_back(errmsg);
-                if (type == REPORT_ERROR)
+                if (type == PipeWriter::REPORT_ERROR)
                     mErrorLogger.reportErr(msg);
                 else
                     mErrorLogger.reportInfo(msg);
             }
         }
-    } else if (type == CHILD_END) {
+    } else if (type == PipeWriter::CHILD_END) {
         std::istringstream iss(buf);
         unsigned int fileResult = 0;
         iss >> fileResult;
@@ -231,9 +276,9 @@ unsigned int ThreadExecutor::check()
                 prctl(PR_SET_PDEATHSIG, SIGHUP);
 #endif
                 close(pipes[0]);
-                mWpipe = pipes[1];
 
-                CppCheck fileChecker(*this, false, CppCheckExecutor::executeCommand);
+                PipeWriter pipewriter(pipes[1]);
+                CppCheck fileChecker(pipewriter, false, CppCheckExecutor::executeCommand);
                 fileChecker.settings() = mSettings;
                 unsigned int resultOfCheck = 0;
 
@@ -249,7 +294,7 @@ unsigned int ThreadExecutor::check()
 
                 std::ostringstream oss;
                 oss << resultOfCheck;
-                writeToPipe(CHILD_END, oss.str());
+                pipewriter.writeEnd(oss.str());
                 std::exit(EXIT_SUCCESS);
             }
 
@@ -341,48 +386,6 @@ unsigned int ThreadExecutor::check()
     return result;
 }
 
-void ThreadExecutor::writeToPipe(PipeSignal type, const std::string &data)
-{
-    unsigned int len = static_cast<unsigned int>(data.length() + 1);
-    char *out = new char[len + 1 + sizeof(len)];
-    out[0] = static_cast<char>(type);
-    std::memcpy(&(out[1]), &len, sizeof(len));
-    std::memcpy(&(out[1+sizeof(len)]), data.c_str(), len);
-    if (write(mWpipe, out, len + 1 + sizeof(len)) <= 0) {
-        delete[] out;
-        out = nullptr;
-        std::cerr << "#### ThreadExecutor::writeToPipe, Failed to write to pipe" << std::endl;
-        std::exit(EXIT_FAILURE);
-    }
-
-    delete[] out;
-}
-
-void ThreadExecutor::reportOut(const std::string &outmsg, Color c)
-{
-    writeToPipe(REPORT_OUT, ::toString(c) + outmsg + ::toString(Color::Reset));
-}
-
-void ThreadExecutor::bughuntingReport(const std::string &str)
-{
-    writeToPipe(REPORT_VERIFICATION, str);
-}
-
-void ThreadExecutor::report(const ErrorMessage &msg, MessageType msgType)
-{
-    PipeSignal pipeSignal;
-    switch (msgType) {
-    case MessageType::REPORT_ERROR:
-        pipeSignal = REPORT_ERROR;
-        break;
-    case MessageType::REPORT_INFO:
-        pipeSignal = REPORT_INFO;
-        break;
-    }
-
-    writeToPipe(pipeSignal, msg.serialize());
-}
-
 void ThreadExecutor::reportInternalChildErr(const std::string &childname, const std::string &msg)
 {
     std::list<ErrorMessage::FileLocation> locations;
@@ -400,25 +403,101 @@ void ThreadExecutor::reportInternalChildErr(const std::string &childname, const
 
 #elif defined(THREADING_MODEL_WIN)
 
+class ThreadExecutor::LogWriter : public ErrorLogger
+{
+public:
+    LogWriter(ThreadExecutor &threadExecutor)
+        : mThreadExecutor(threadExecutor), mProcessedFiles(0), mTotalFiles(0), mProcessedSize(0), mTotalFileSize(0) {
+
+        mItNextFile = threadExecutor.mFiles.begin();
+        mItNextFileSettings = threadExecutor.mSettings.project.fileSettings.begin();
+
+        mTotalFiles = threadExecutor.mFiles.size() + threadExecutor.mSettings.project.fileSettings.size();
+        for (std::map<std::string, std::size_t>::const_iterator i = threadExecutor.mFiles.begin(); i != threadExecutor.mFiles.end(); ++i) {
+            mTotalFileSize += i->second;
+        }
+    }
+
+    void reportOut(const std::string &outmsg, Color c) override
+    {
+        std::lock_guard<std::mutex> lg(mReportSync);
+
+        mThreadExecutor.mErrorLogger.reportOut(outmsg, c);
+    }
+
+    void reportErr(const ErrorMessage &msg) override {
+        report(msg, MessageType::REPORT_ERROR);
+    }
+
+    void reportInfo(const ErrorMessage &msg) override {
+        report(msg, MessageType::REPORT_INFO);
+    }
+
+    void bughuntingReport(const std::string &str) override
+    {
+        std::lock_guard<std::mutex> lg(mReportSync);
+        mThreadExecutor.mErrorLogger.bughuntingReport(str);
+    }
+
+    ThreadExecutor &mThreadExecutor;
+
+    std::map<std::string, std::size_t>::const_iterator mItNextFile;
+    std::list<ImportProject::FileSettings>::const_iterator mItNextFileSettings;
+
+    std::size_t mProcessedFiles;
+    std::size_t mTotalFiles;
+    std::size_t mProcessedSize;
+    std::size_t mTotalFileSize;
+
+    std::mutex mFileSync;
+    std::mutex mErrorSync;
+    std::mutex mReportSync;
+
+private:
+    enum class MessageType {REPORT_ERROR, REPORT_INFO};
+
+    void report(const ErrorMessage &msg, MessageType msgType)
+    {
+        if (mThreadExecutor.mSettings.nomsg.isSuppressed(msg.toSuppressionsErrorMessage()))
+            return;
+
+        // Alert only about unique errors
+        bool reportError = false;
+        const std::string errmsg = msg.toString(mThreadExecutor.mSettings.verbose);
+
+        {
+            std::lock_guard<std::mutex> lg(mErrorSync);
+            if (std::find(mThreadExecutor.mErrorList.begin(), mThreadExecutor.mErrorList.end(), errmsg) == mThreadExecutor.mErrorList.end()) {
+                mThreadExecutor.mErrorList.emplace_back(errmsg);
+                reportError = true;
+            }
+        }
+
+        if (reportError) {
+            std::lock_guard<std::mutex> lg(mReportSync);
+
+            switch (msgType) {
+            case MessageType::REPORT_ERROR:
+                mThreadExecutor.mErrorLogger.reportErr(msg);
+                break;
+            case MessageType::REPORT_INFO:
+                mThreadExecutor.mErrorLogger.reportInfo(msg);
+                break;
+            }
+        }
+    }
+};
+
 unsigned int ThreadExecutor::check()
 {
     std::vector<std::future<unsigned int>> threadFutures;
     threadFutures.reserve(mSettings.jobs);
 
-    mItNextFile = mFiles.begin();
+    LogWriter logwriter(*this);
-    mItNextFileSettings = mSettings.project.fileSettings.begin();
-
-    mProcessedFiles = 0;
-    mProcessedSize = 0;
-    mTotalFiles = mFiles.size() + mSettings.project.fileSettings.size();
-    mTotalFileSize = 0;
-    for (std::map<std::string, std::size_t>::const_iterator i = mFiles.begin(); i != mFiles.end(); ++i) {
-        mTotalFileSize += i->second;
-    }
 
     for (unsigned int i = 0; i < mSettings.jobs; ++i) {
         try {
-            threadFutures.emplace_back(std::async(std::launch::async, threadProc, this));
+            threadFutures.emplace_back(std::async(std::launch::async, threadProc, &logwriter));
         }
         catch (const std::system_error &e) {
             std::cerr << "#### ThreadExecutor::check exception :" << e.what() << std::endl;
@@ -431,35 +510,35 @@ unsigned int ThreadExecutor::check()
     });
 }
 
-unsigned int __stdcall ThreadExecutor::threadProc(ThreadExecutor* threadExecutor)
+unsigned int __stdcall ThreadExecutor::threadProc(LogWriter* logWriter)
 {
     unsigned int result = 0;
 
-    std::map<std::string, std::size_t>::const_iterator &itFile = threadExecutor->mItNextFile;
+    std::map<std::string, std::size_t>::const_iterator &itFile = logWriter->mItNextFile;
-    std::list<ImportProject::FileSettings>::const_iterator &itFileSettings = threadExecutor->mItNextFileSettings;
+    std::list<ImportProject::FileSettings>::const_iterator &itFileSettings = logWriter->mItNextFileSettings;
 
     // guard static members of CppCheck against concurrent access
-    threadExecutor->mFileSync.lock();
+    logWriter->mFileSync.lock();
 
     for (;;) {
-        if (itFile == threadExecutor->mFiles.end() && itFileSettings == threadExecutor->mSettings.project.fileSettings.end()) {
+        if (itFile == logWriter->mThreadExecutor.mFiles.end() && itFileSettings == logWriter->mThreadExecutor.mSettings.project.fileSettings.end()) {
-            threadExecutor->mFileSync.unlock();
+            logWriter->mFileSync.unlock();
             break;
         }
 
-        CppCheck fileChecker(*threadExecutor, false, CppCheckExecutor::executeCommand);
+        CppCheck fileChecker(*logWriter, false, CppCheckExecutor::executeCommand);
-        fileChecker.settings() = threadExecutor->mSettings;
+        fileChecker.settings() = logWriter->mThreadExecutor.mSettings;
 
         std::size_t fileSize = 0;
-        if (itFile != threadExecutor->mFiles.end()) {
+        if (itFile != logWriter->mThreadExecutor.mFiles.end()) {
             const std::string &file = itFile->first;
             fileSize = itFile->second;
             ++itFile;
 
-            threadExecutor->mFileSync.unlock();
+            logWriter->mFileSync.unlock();
 
-            const std::map<std::string, std::string>::const_iterator fileContent = threadExecutor->mFileContents.find(file);
+            const std::map<std::string, std::string>::const_iterator fileContent = logWriter->mThreadExecutor.mFileContents.find(file);
-            if (fileContent != threadExecutor->mFileContents.end()) {
+            if (fileContent != logWriter->mThreadExecutor.mFileContents.end()) {
                 // File content was given as a string
                 result += fileChecker.check(file, fileContent->second);
             } else {
@@ -469,64 +548,21 @@ unsigned int __stdcall ThreadExecutor::threadProc(ThreadExecutor* threadExecutor
         } else { // file settings..
             const ImportProject::FileSettings &fs = *itFileSettings;
             ++itFileSettings;
-            threadExecutor->mFileSync.unlock();
+            logWriter->mFileSync.unlock();
             result += fileChecker.check(fs);
-            if (threadExecutor->mSettings.clangTidy)
+            if (logWriter->mThreadExecutor.mSettings.clangTidy)
                 fileChecker.analyseClangTidy(fs);
         }
 
-        threadExecutor->mFileSync.lock();
+        logWriter->mFileSync.lock();
 
-        threadExecutor->mProcessedSize += fileSize;
+        logWriter->mProcessedSize += fileSize;
-        threadExecutor->mProcessedFiles++;
+        logWriter->mProcessedFiles++;
-        if (!threadExecutor->mSettings.quiet) {
+        if (!logWriter->mThreadExecutor.mSettings.quiet) {
-            std::lock_guard<std::mutex> lg(threadExecutor->mReportSync);
+            std::lock_guard<std::mutex> lg(logWriter->mReportSync);
-            CppCheckExecutor::reportStatus(threadExecutor->mProcessedFiles, threadExecutor->mTotalFiles, threadExecutor->mProcessedSize, threadExecutor->mTotalFileSize);
+            CppCheckExecutor::reportStatus(logWriter->mProcessedFiles, logWriter->mTotalFiles, logWriter->mProcessedSize, logWriter->mTotalFileSize);
         }
     }
     return result;
 }
-
-void ThreadExecutor::reportOut(const std::string &outmsg, Color c)
-{
-    std::lock_guard<std::mutex> lg(mReportSync);
-
-    mErrorLogger.reportOut(outmsg, c);
-}
-
-void ThreadExecutor::bughuntingReport(const std::string  & /*str*/)
-{
-    // TODO
-}
-
-void ThreadExecutor::report(const ErrorMessage &msg, MessageType msgType)
-{
-    if (mSettings.nomsg.isSuppressed(msg.toSuppressionsErrorMessage()))
-        return;
-
-    // Alert only about unique errors
-    bool reportError = false;
-    const std::string errmsg = msg.toString(mSettings.verbose);
-
-    {
-        std::lock_guard<std::mutex> lg(mErrorSync);
-        if (std::find(mErrorList.begin(), mErrorList.end(), errmsg) == mErrorList.end()) {
-            mErrorList.emplace_back(errmsg);
-            reportError = true;
-        }
-    }
-
-    if (reportError) {
-        std::lock_guard<std::mutex> lg(mReportSync);
-
-        switch (msgType) {
-        case MessageType::REPORT_ERROR:
-            mErrorLogger.reportErr(msg);
-            break;
-        case MessageType::REPORT_INFO:
-            mErrorLogger.reportInfo(msg);
-            break;
-        }
-    }
-}
 #endif

cli/threadexecutor.h

@@ -19,9 +19,6 @@
 #ifndef THREADEXECUTOR_H
 #define THREADEXECUTOR_H
 
-#include "color.h"
-#include "errorlogger.h"
-
 #include <cstddef>
 #include <list>
 #include <map>
@@ -31,13 +28,12 @@
 #define THREADING_MODEL_FORK
 #elif defined(_WIN32)
 #define THREADING_MODEL_WIN
-#include "importproject.h"
-#include <mutex>
 #else
-#error "No threading moodel defined"
+#error "No threading model defined"
 #endif
 
 class Settings;
+class ErrorLogger;
 
 /// @addtogroup CLI
 /// @{
@@ -46,19 +42,14 @@ class Settings;
  * This class will take a list of filenames and settings and check then
  * all files using threads.
  */
-class ThreadExecutor : public ErrorLogger {
+class ThreadExecutor {
 public:
     ThreadExecutor(const std::map<std::string, std::size_t> &files, Settings &settings, ErrorLogger &errorLogger);
     ThreadExecutor(const ThreadExecutor &) = delete;
-    ~ThreadExecutor() override;
+    ~ThreadExecutor();
     void operator=(const ThreadExecutor &) = delete;
     unsigned int check();
 
-    void reportOut(const std::string &outmsg, Color c) override;
-    void reportErr(const ErrorMessage &msg) override;
-    void reportInfo(const ErrorMessage &msg) override;
-    void bughuntingReport(const std::string &str) override;
-
     /**
      * @brief Add content to a file, to be used in unit testing.
      *
@@ -69,21 +60,16 @@ public:
     void addFileContent(const std::string &path, const std::string &content);
 
 private:
-    enum class MessageType {REPORT_ERROR, REPORT_INFO};
-
     const std::map<std::string, std::size_t> &mFiles;
     Settings &mSettings;
     ErrorLogger &mErrorLogger;
     unsigned int mFileCount;
-
+    std::list<std::string> mErrorList;
-    void report(const ErrorMessage &msg, MessageType msgType);
-
-#if defined(THREADING_MODEL_FORK)
 
     /** @brief Key is file name, and value is the content of the file */
     std::map<std::string, std::string> mFileContents;
-private:
+
-    enum PipeSignal {REPORT_OUT='1',REPORT_ERROR='2', REPORT_INFO='3', REPORT_VERIFICATION='4', CHILD_END='5'};
+#if defined(THREADING_MODEL_FORK)
 
     /**
      * Read from the pipe, parse and handle what ever is in there.
@@ -92,13 +78,6 @@ private:
      *         1 if we did read something
      */
     int handleRead(int rpipe, unsigned int &result);
-    void writeToPipe(PipeSignal type, const std::string &data);
-    /**
-     * Write end of status pipe, different for each child.
-     * Not used in master process.
-     */
-    std::list<std::string> mErrorList;
-    int mWpipe;
 
     /**
      * @brief Check load average condition
@@ -113,32 +92,12 @@ private:
      */
     void reportInternalChildErr(const std::string &childname, const std::string &msg);
 
-public:
-    /**
-     * @return true if support for threads exist.
-     */
-    static bool isEnabled() {
-        return true;
-    }
-
 #elif defined(THREADING_MODEL_WIN)
 
-private:
+    class LogWriter;
-    std::map<std::string, std::string> mFileContents;
+    static unsigned __stdcall threadProc(LogWriter *threadExecutor);
-    std::map<std::string, std::size_t>::const_iterator mItNextFile;
-    std::list<ImportProject::FileSettings>::const_iterator mItNextFileSettings;
-    std::size_t mProcessedFiles;
-    std::size_t mTotalFiles;
-    std::size_t mProcessedSize;
-    std::size_t mTotalFileSize;
-    std::mutex mFileSync;
 
-    std::list<std::string> mErrorList;
+#endif
-    std::mutex mErrorSync;
-
-    std::mutex mReportSync;
-
-    static unsigned __stdcall threadProc(ThreadExecutor *threadExecutor);
 
 public:
     /**
@@ -147,15 +106,7 @@ public:
     static bool isEnabled() {
         return true;
     }
-#else
+
-public:
-    /**
-     * @return true if support for threads exist.
-     */
-    static bool isEnabled() {
-        return false;
-    }
-#endif
 };
 
 /// @}