ClangToolRunner: Add done(const AnalyzeOutputData &) signal

Introduce AnalyzerOutputData structure that is passed inside
new done() signal. This signal replaces the finishedWithSuccess()
and finishedWithFailure() signals. The output structure contains
all the data required in clients' handlers.

Move AnalyzeUnit into clangtoolrunner.h in order to avoid
circular dependencies.

Get rid of outputFilePath(), as it's passed inside AnalyzeOutputData
now.

Inline ClangToolRunWorker::unitsToAnalyze() as it's used only once.

Change-Id: Icf9a52853c68e83f6ddfc4858dbcb830b96e1844
Reviewed-by: <[email protected]>
Reviewed-by: David Schulz <[email protected]>

7 files changed, 123 insertions, 159 deletions

diff --git a/src/plugins/clangtools/clangtoolruncontrol.cpp b/src/plugins/clangtools/clangtoolruncontrol.cpp
index ab3122fad8c..b4320610017 100644
--- a/src/plugins/clangtools/clangtoolruncontrol.cpp
+++ b/src/plugins/clangtools/clangtoolruncontrol.cpp
@@ -6,14 +6,12 @@
 #include "clangtool.h"
 #include "clangtoolrunner.h"
 #include "clangtoolssettings.h"
-#include "clangtoolsutils.h"
 #include "executableinfo.h"
 
 #include <debugger/analyzer/analyzerconstants.h>
 
 #include <clangcodemodel/clangutils.h>
 
-#include <coreplugin/icore.h>
 #include <coreplugin/progressmanager/futureprogress.h>
 #include <coreplugin/progressmanager/progressmanager.h>
 
@@ -21,7 +19,6 @@
 #include <cppeditor/compileroptionsbuilder.h>
 #include <cppeditor/cppmodelmanager.h>
 #include <cppeditor/cppprojectfile.h>
-#include <cppeditor/cpptoolsreuse.h>
 #include <cppeditor/projectinfo.h>
 
 #include <projectexplorer/abi.h>
@@ -87,35 +84,6 @@ private:
      bool m_success = false;
 };
 
-AnalyzeUnit::AnalyzeUnit(const FileInfo &fileInfo,
-                         const FilePath &clangIncludeDir,
-                         const QString &clangVersion)
-{
-    const FilePath actualClangIncludeDir = Core::ICore::clangIncludeDirectory(
-                clangVersion, clangIncludeDir);
-    CompilerOptionsBuilder optionsBuilder(*fileInfo.projectPart,
-                                          UseSystemHeader::No,
-                                          UseTweakedHeaderPaths::Tools,
-                                          UseLanguageDefines::No,
-                                          UseBuildSystemWarnings::No,
-                                          actualClangIncludeDir);
-    file = fileInfo.file.toString();
-    arguments = extraClangToolsPrependOptions();
-    arguments.append(optionsBuilder.build(fileInfo.kind, CppEditor::getPchUsage()));
-    arguments.append(extraClangToolsAppendOptions());
-}
-
-AnalyzeUnits ClangToolRunWorker::unitsToAnalyze(const FilePath &clangIncludeDir,
-                                                const QString &clangVersion)
-{
-    QTC_ASSERT(m_projectInfo, return AnalyzeUnits());
-
-    AnalyzeUnits units;
-    for (const FileInfo &fileInfo : m_fileInfos)
-        units << AnalyzeUnit(fileInfo, clangIncludeDir, clangVersion);
-    return units;
-}
-
 static QDebug operator<<(QDebug debug, const Utils::Environment &environment)
 {
     for (const QString &entry : environment.toStringList())
@@ -218,16 +186,19 @@ void ClangToolRunWorker::start()
                   Utils::NormalMessageFormat);
 
     // Collect files
-    const auto clangIncludeDirAndVersion =
-            getClangIncludeDirAndVersion(runControl()->commandLine().executable());
-    const AnalyzeUnits unitsToProcess = unitsToAnalyze(clangIncludeDirAndVersion.first,
-                                                       clangIncludeDirAndVersion.second);
+    const auto [includeDir, clangVersion]
+        = getClangIncludeDirAndVersion(runControl()->commandLine().executable());
+
+    AnalyzeUnits unitsToProcess;
+    for (const FileInfo &fileInfo : m_fileInfos)
+        unitsToProcess.append({fileInfo, includeDir, clangVersion});
+
     qCDebug(LOG) << Q_FUNC_INFO << runControl()->commandLine().executable()
-                 << clangIncludeDirAndVersion.first << clangIncludeDirAndVersion.second;
+                 << includeDir << clangVersion;
     qCDebug(LOG) << "Files to process:" << unitsToProcess;
 
     m_runnerCreators.clear();
-    for (const AnalyzeUnit &unit : unitsToProcess) {
+    for (const AnalyzeUnit &unit : std::as_const(unitsToProcess)) {
         for (const RunnerCreator &creator : runnerCreators(unit))
             m_runnerCreators << creator;
     }
@@ -308,27 +279,38 @@ void ClangToolRunWorker::analyzeNextFile()
     }
 }
 
-void ClangToolRunWorker::onRunnerFinishedWithSuccess(ClangToolRunner *runner,
-                                                     const QString &filePath)
+void ClangToolRunWorker::onDone(const AnalyzeOutputData &output)
 {
-    const QString outputFilePath = runner->outputFilePath();
-    qCDebug(LOG) << "onRunnerFinishedWithSuccess:" << outputFilePath;
-
     emit runnerFinished();
 
+    if (!output.success) {
+        qCDebug(LOG).noquote() << "onRunnerFinishedWithFailure:" << output.errorMessage << '\n'
+                               << output.errorDetails;
+        m_filesAnalyzed.remove(output.fileToAnalyze);
+        m_filesNotAnalyzed.insert(output.fileToAnalyze);
+
+        const QString message = tr("Failed to analyze \"%1\": %2")
+                                    .arg(output.fileToAnalyze, output.errorMessage);
+        appendMessage(message, Utils::StdErrFormat);
+        appendMessage(output.errorDetails, Utils::StdErrFormat);
+        return;
+    }
+
+    qCDebug(LOG) << "onRunnerFinishedWithSuccess:" << output.outputFilePath;
+
     QString errorMessage;
-    const Diagnostics diagnostics = m_tool->read(outputFilePath, m_projectFiles, &errorMessage);
+    const Diagnostics diagnostics = m_tool->read(output.outputFilePath, m_projectFiles,
+                                                 &errorMessage);
 
     if (!errorMessage.isEmpty()) {
-        m_filesAnalyzed.remove(filePath);
-        m_filesNotAnalyzed.insert(filePath);
+        m_filesAnalyzed.remove(output.fileToAnalyze);
+        m_filesNotAnalyzed.insert(output.fileToAnalyze);
         qCDebug(LOG) << "onRunnerFinishedWithSuccess: Error reading log file:" << errorMessage;
-        const QString filePath = runner->fileToAnalyze();
-        appendMessage(tr("Failed to analyze \"%1\": %2").arg(filePath, errorMessage),
+        appendMessage(tr("Failed to analyze \"%1\": %2").arg(output.fileToAnalyze, errorMessage),
                       Utils::StdErrFormat);
     } else {
-        if (!m_filesNotAnalyzed.contains(filePath))
-            m_filesAnalyzed.insert(filePath);
+        if (!m_filesNotAnalyzed.contains(output.fileToAnalyze))
+            m_filesAnalyzed.insert(output.fileToAnalyze);
         if (!diagnostics.isEmpty()) {
             // do not generate marks when we always analyze open files since marks from that
             // analysis should be more up to date
@@ -336,28 +318,6 @@ void ClangToolRunWorker::onRunnerFinishedWithSuccess(ClangToolRunner *runner,
             m_tool->onNewDiagnosticsAvailable(diagnostics, generateMarks);
         }
     }
-
-    handleFinished(runner);
-}
-
-void ClangToolRunWorker::onRunnerFinishedWithFailure(ClangToolRunner *runner,
-                                                     const QString &errorMessage,
-                                                     const QString &errorDetails)
-{
-    qCDebug(LOG).noquote() << "onRunnerFinishedWithFailure:" << errorMessage
-                           << '\n' << errorDetails;
-
-    emit runnerFinished();
-
-    const QString fileToAnalyze = runner->fileToAnalyze();
-
-    m_filesAnalyzed.remove(fileToAnalyze);
-    m_filesNotAnalyzed.insert(fileToAnalyze);
-
-    const QString message = tr("Failed to analyze \"%1\": %2").arg(fileToAnalyze, errorMessage);
-    appendMessage(message, Utils::StdErrFormat);
-    appendMessage(errorDetails, Utils::StdErrFormat);
-    handleFinished(runner);
 }
 
 void ClangToolRunWorker::handleFinished(ClangToolRunner *runner)
@@ -408,13 +368,12 @@ void ClangToolRunWorker::finalize()
 
 ClangToolRunner *ClangToolRunWorker::createRunner(ClangToolType tool, const AnalyzeUnit &unit)
 {
-    using namespace std::placeholders;
     auto runner = new ClangToolRunner(
         {tool, m_diagnosticConfig, m_temporaryDir.path(), m_environment, unit}, this);
-    connect(runner, &ClangToolRunner::finishedWithSuccess, this,
-            std::bind(&ClangToolRunWorker::onRunnerFinishedWithSuccess, this, runner, _1));
-    connect(runner, &ClangToolRunner::finishedWithFailure, this,
-            std::bind(&ClangToolRunWorker::onRunnerFinishedWithFailure, this, runner, _1, _2));
+    connect(runner, &ClangToolRunner::done, this, [this, runner](const AnalyzeOutputData &output) {
+        onDone(output);
+        handleFinished(runner);
+    });
     return runner;
 }
 
diff --git a/src/plugins/clangtools/clangtoolruncontrol.h b/src/plugins/clangtools/clangtoolruncontrol.h
index 019613f88d4..08ba1806021 100644
--- a/src/plugins/clangtools/clangtoolruncontrol.h
+++ b/src/plugins/clangtools/clangtoolruncontrol.h
@@ -19,20 +19,13 @@
 
 namespace ClangTools {
 namespace Internal {
+
+class AnalyzeOutputData;
+class AnalyzeUnit;
 class ClangTool;
 class ClangToolRunner;
 class ProjectBuilder;
 
-struct AnalyzeUnit {
-    AnalyzeUnit(const FileInfo &fileInfo,
-                const Utils::FilePath &clangResourceDir,
-                const QString &clangVersion);
-
-    QString file;
-    QStringList arguments; // without file itself and "-o somePath"
-};
-using AnalyzeUnits = QList<AnalyzeUnit>;
-
 using RunnerCreator = std::function<ClangToolRunner*()>;
 
 class ClangToolRunWorker : public ProjectExplorer::RunWorker
@@ -56,22 +49,15 @@ signals:
     void runnerFinished();
     void startFailed();
 
-protected:
-    void onRunnerFinishedWithSuccess(ClangToolRunner *runner, const QString &filePath);
-    void onRunnerFinishedWithFailure(ClangToolRunner *runner, const QString &errorMessage,
-                                     const QString &errorDetails);
-
 private:
     void start() final;
     void stop() final;
+    void onDone(const AnalyzeOutputData &output);
 
     QList<RunnerCreator> runnerCreators(const AnalyzeUnit &unit);
     ClangToolRunner *createRunner(CppEditor::ClangToolType tool, const AnalyzeUnit &unit);
 
-    AnalyzeUnits unitsToAnalyze(const Utils::FilePath &clangIncludeDir,
-                                const QString &clangVersion);
     void analyzeNextFile();
-
     void handleFinished(ClangToolRunner *runner);
 
     void onProgressCanceled();
diff --git a/src/plugins/clangtools/clangtoolrunner.cpp b/src/plugins/clangtools/clangtoolrunner.cpp
index dd0cfd41897..59f8d4c322b 100644
--- a/src/plugins/clangtools/clangtoolrunner.cpp
+++ b/src/plugins/clangtools/clangtoolrunner.cpp
@@ -5,8 +5,11 @@
 
 #include "clangtoolsutils.h"
 
+#include <coreplugin/icore.h>
+
 #include <cppeditor/clangdiagnosticconfigsmodel.h>
 #include <cppeditor/compileroptionsbuilder.h>
+#include <cppeditor/cpptoolsreuse.h>
 
 #include <utils/environment.h>
 #include <utils/qtcassert.h>
@@ -26,6 +29,24 @@ using namespace Utils;
 namespace ClangTools {
 namespace Internal {
 
+AnalyzeUnit::AnalyzeUnit(const FileInfo &fileInfo,
+                         const FilePath &clangIncludeDir,
+                         const QString &clangVersion)
+{
+    const FilePath actualClangIncludeDir = Core::ICore::clangIncludeDirectory(
+        clangVersion, clangIncludeDir);
+    CompilerOptionsBuilder optionsBuilder(*fileInfo.projectPart,
+                                          UseSystemHeader::No,
+                                          UseTweakedHeaderPaths::Tools,
+                                          UseLanguageDefines::No,
+                                          UseBuildSystemWarnings::No,
+                                          actualClangIncludeDir);
+    file = fileInfo.file.toString();
+    arguments = extraClangToolsPrependOptions();
+    arguments.append(optionsBuilder.build(fileInfo.kind, CppEditor::getPchUsage()));
+    arguments.append(extraClangToolsAppendOptions());
+}
+
 static bool isClMode(const QStringList &options)
 {
     return options.contains("--driver-mode=cl");
@@ -64,21 +85,6 @@ static QStringList clangArguments(const ClangDiagnosticConfig &diagnosticConfig,
     return arguments;
 }
 
-static QString generalProcessError(const QString &name)
-{
-    return ClangToolRunner::tr("An error occurred with the %1 process.").arg(name);
-}
-
-static QString finishedDueToCrash(const QString &name)
-{
-    return ClangToolRunner::tr("%1 crashed.").arg(name);
-}
-
-static QString finishedWithBadExitCode(const QString &name, int exitCode)
-{
-    return ClangToolRunner::tr("%1 finished with exit code: %2.").arg(name).arg(exitCode);
-}
-
 ClangToolRunner::ClangToolRunner(const AnalyzeInputData &input, QObject *parent)
     : QObject(parent)
     , m_input(input)
@@ -156,27 +162,27 @@ bool ClangToolRunner::run()
 
 void ClangToolRunner::onProcessDone()
 {
-    if (m_process.result() == ProcessResult::StartFailed) {
-        emit finishedWithFailure(generalProcessError(m_name), commandlineAndOutput());
-    } else if (m_process.result() == ProcessResult::FinishedWithSuccess) {
+    if (m_process.result() == ProcessResult::FinishedWithSuccess) {
         qCDebug(LOG).noquote() << "Output:\n" << m_process.cleanedStdOut();
-        emit finishedWithSuccess(m_input.unit.file);
-    } else if (m_process.result() == ProcessResult::FinishedWithError) {
-        emit finishedWithFailure(finishedWithBadExitCode(m_name, m_process.exitCode()),
-                                 commandlineAndOutput());
-    } else { // == QProcess::CrashExit
-        emit finishedWithFailure(finishedDueToCrash(m_name), commandlineAndOutput());
+        emit done({true, m_input.unit.file, m_outputFilePath, m_name});
+        return;
     }
-}
 
-QString ClangToolRunner::commandlineAndOutput() const
-{
-    return tr("Command line: %1\n"
-              "Process Error: %2\n"
-              "Output:\n%3")
-        .arg(m_process.commandLine().toUserOutput())
-        .arg(m_process.error())
-        .arg(m_process.cleanedStdOut());
+    const QString details = tr("Command line: %1\n"
+                               "Process Error: %2\n"
+                               "Output:\n%3")
+                                .arg(m_process.commandLine().toUserOutput())
+                                .arg(m_process.error())
+                                .arg(m_process.cleanedStdOut());
+    QString message;
+    if (m_process.result() == ProcessResult::StartFailed)
+        message = tr("An error occurred with the %1 process.").arg(m_name);
+    else if (m_process.result() == ProcessResult::FinishedWithError)
+        message = tr("%1 finished with exit code: %2.").arg(m_name).arg(m_process.exitCode());
+    else
+        message = tr("%1 crashed.").arg(m_name);
+
+    emit done({false, m_input.unit.file, m_outputFilePath, m_name, message, details});
 }
 
 } // namespace Internal
diff --git a/src/plugins/clangtools/clangtoolrunner.h b/src/plugins/clangtools/clangtoolrunner.h
index 87ddc45d970..681480e6430 100644
--- a/src/plugins/clangtools/clangtoolrunner.h
+++ b/src/plugins/clangtools/clangtoolrunner.h
@@ -3,7 +3,7 @@
 
 #pragma once
 
-#include "clangtoolruncontrol.h"
+#include "clangfileinfo.h"
 
 #include <cppeditor/clangdiagnosticconfig.h>
 
@@ -15,6 +15,16 @@
 namespace ClangTools {
 namespace Internal {
 
+struct AnalyzeUnit {
+    AnalyzeUnit(const FileInfo &fileInfo,
+                const Utils::FilePath &clangResourceDir,
+                const QString &clangVersion);
+
+    QString file;
+    QStringList arguments; // without file itself and "-o somePath"
+};
+using AnalyzeUnits = QList<AnalyzeUnit>;
+
 struct AnalyzeInputData
 {
     CppEditor::ClangToolType tool = CppEditor::ClangToolType::Tidy;
@@ -24,6 +34,17 @@ struct AnalyzeInputData
     AnalyzeUnit unit;
     QString overlayFilePath = {};
 };
+
+struct AnalyzeOutputData
+{
+    bool success = true;
+    QString fileToAnalyze;
+    QString outputFilePath;
+    QString toolName;
+    QString errorMessage = {};
+    QString errorDetails = {};
+};
+
 class ClangToolRunner : public QObject
 {
     Q_OBJECT
@@ -33,7 +54,6 @@ public:
 
     QString name() const { return m_name; }
     QString fileToAnalyze() const { return m_input.unit.file; }
-    QString outputFilePath() const { return m_outputFilePath; }
     bool supportsVFSOverlay() const;
 
     // compilerOptions is expected to contain everything except:
@@ -42,22 +62,18 @@ public:
     bool run();
 
 signals:
-    void finishedWithSuccess(const QString &fileToAnalyze);
-    void finishedWithFailure(const QString &errorMessage, const QString &errorDetails);
+    void done(const AnalyzeOutputData &output);
 
 private:
-    void onProcessOutput();
     void onProcessDone();
 
     QStringList mainToolArguments() const;
-    QString commandlineAndOutput() const;
 
     const AnalyzeInputData m_input;
     Utils::QtcProcess m_process;
 
     QString m_name;
     Utils::FilePath m_executable;
-
     QString m_outputFilePath;
 };
 
diff --git a/src/plugins/clangtools/clangtoolslogfilereader.h b/src/plugins/clangtools/clangtoolslogfilereader.h
index 298fef35451..4b8f3edcc1e 100644
--- a/src/plugins/clangtools/clangtoolslogfilereader.h
+++ b/src/plugins/clangtools/clangtoolslogfilereader.h
@@ -18,7 +18,7 @@ using AcceptDiagsFromFilePath = std::function<bool(const Utils::FilePath &)>;
 // Reads diagnostics generated by "clang-tidy/clazy-standalone -export-fixes=path/to/file"
 Diagnostics readExportedDiagnostics(const Utils::FilePath &logFilePath,
                                     const AcceptDiagsFromFilePath &acceptFromFilePath,
-                                    QString *errorMessage);
+                                    QString *errorMessage = nullptr);
 
 // Exposed for tests
 struct LineColumnInfo {
diff --git a/src/plugins/clangtools/documentclangtoolrunner.cpp b/src/plugins/clangtools/documentclangtoolrunner.cpp
index 2d2bd2fb9ef..a15809ae19b 100644
--- a/src/plugins/clangtools/documentclangtoolrunner.cpp
+++ b/src/plugins/clangtools/documentclangtoolrunner.cpp
@@ -242,14 +242,19 @@ static void updateLocation(Debugger::DiagnosticLocation &location)
     location.filePath = vfso().originalFilePath(location.filePath);
 }
 
-void DocumentClangToolRunner::onSuccess()
+void DocumentClangToolRunner::onDone(const AnalyzeOutputData &output)
 {
-    QString errorMessage;
-    FilePath mappedPath = vfso().autoSavedFilePath(m_document);
+    if (!output.success) {
+        qCDebug(LOG) << "Failed to analyze " << m_fileInfo.file
+                     << ":" << output.errorMessage << output.errorDetails;
+        runNext();
+        return;
+    }
+
+    const FilePath mappedPath = vfso().autoSavedFilePath(m_document);
     Diagnostics diagnostics = readExportedDiagnostics(
-        FilePath::fromString(m_currentRunner->outputFilePath()),
-        [&](const FilePath &path) { return path == mappedPath; },
-        &errorMessage);
+        FilePath::fromString(output.outputFilePath),
+        [&](const FilePath &path) { return path == mappedPath; });
 
     for (Diagnostic &diag : diagnostics) {
         updateLocation(diag.location);
@@ -260,9 +265,10 @@ void DocumentClangToolRunner::onSuccess()
         }
     }
 
+    const QString toolName = output.toolName;
     // remove outdated marks of the current runner
-    auto [toDelete, newMarks] = Utils::partition(m_marks, [this](DiagnosticMark *mark) {
-        return mark->source == m_currentRunner->name();
+    auto [toDelete, newMarks] = Utils::partition(m_marks, [toolName](DiagnosticMark *mark) {
+        return mark->source == toolName; // TODO: comparison on translatable string
     });
     m_marks = newMarks;
     qDeleteAll(toDelete);
@@ -271,12 +277,12 @@ void DocumentClangToolRunner::onSuccess()
 
     TextEditor::RefactorMarkers markers;
 
-    for (const Diagnostic &diagnostic : diagnostics) {
+    for (const Diagnostic &diagnostic : std::as_const(diagnostics)) {
         if (isSuppressed(diagnostic))
             continue;
 
         auto mark = new DiagnosticMark(diagnostic);
-        mark->source = m_currentRunner->name();
+        mark->source = toolName;
 
         if (doc && Utils::anyOf(diagnostic.explainingSteps, &ExplainingStep::isFixIt)) {
             TextEditor::RefactorMarker marker;
@@ -309,12 +315,6 @@ void DocumentClangToolRunner::onSuccess()
     runNext();
 }
 
-void DocumentClangToolRunner::onFailure(const QString &errorMessage, const QString &errorDetails)
-{
-    qCDebug(LOG) << "Failed to analyze " << m_fileInfo.file << ":" << errorMessage << errorDetails;
-    runNext();
-}
-
 void DocumentClangToolRunner::finalize()
 {
     // remove all disabled textMarks
@@ -350,10 +350,7 @@ ClangToolRunner *DocumentClangToolRunner::createRunner(ClangToolType tool, const
 {
     auto runner = new ClangToolRunner({tool, config, m_temporaryDir.path(), env, unit,
                                        vfso().overlayFilePath().toString()}, this);
-    connect(runner, &ClangToolRunner::finishedWithSuccess,
-            this, &DocumentClangToolRunner::onSuccess);
-    connect(runner, &ClangToolRunner::finishedWithFailure,
-            this, &DocumentClangToolRunner::onFailure);
+    connect(runner, &ClangToolRunner::done, this, &DocumentClangToolRunner::onDone);
     return runner;
 }
 
diff --git a/src/plugins/clangtools/documentclangtoolrunner.h b/src/plugins/clangtools/documentclangtoolrunner.h
index 196c1829be4..1699f35f9d7 100644
--- a/src/plugins/clangtools/documentclangtoolrunner.h
+++ b/src/plugins/clangtools/documentclangtoolrunner.h
@@ -4,7 +4,6 @@
 #pragma once
 
 #include "clangfileinfo.h"
-#include "clangtoolruncontrol.h"
 #include "clangtoolsdiagnostic.h"
 #include "clangtoolsprojectsettings.h"
 
@@ -22,6 +21,8 @@ namespace ClangTools {
 
 namespace Internal {
 
+class AnalyzeOutputData;
+class AnalyzeUnit;
 class ClangToolRunner;
 class DiagnosticMark;
 
@@ -40,8 +41,7 @@ private:
     void run();
     void runNext();
 
-    void onSuccess();
-    void onFailure(const QString &errorMessage, const QString &errorDetails);
+    void onDone(const AnalyzeOutputData &output);
 
     void finalize();