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: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: David Schulz <david.schulz@qt.io>

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 =
+    const auto [includeDir, clangVersion]
-            getClangIncludeDirAndVersion(runControl()->commandLine().executable());
+        = getClangIncludeDirAndVersion(runControl()->commandLine().executable());
-    const AnalyzeUnits unitsToProcess = unitsToAnalyze(clangIncludeDirAndVersion.first,
+
-                                                       clangIncludeDirAndVersion.second);
+    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,
+void ClangToolRunWorker::onDone(const AnalyzeOutputData &output)
-                                                     const QString &filePath)
 {
-    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_filesAnalyzed.remove(output.fileToAnalyze);
-        m_filesNotAnalyzed.insert(filePath);
+        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(output.fileToAnalyze, errorMessage),
-        appendMessage(tr("Failed to analyze \"%1\": %2").arg(filePath, errorMessage),
                       Utils::StdErrFormat);
     } else {
-        if (!m_filesNotAnalyzed.contains(filePath))
+        if (!m_filesNotAnalyzed.contains(output.fileToAnalyze))
-            m_filesAnalyzed.insert(filePath);
+            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,
+    connect(runner, &ClangToolRunner::done, this, [this, runner](const AnalyzeOutputData &output) {
-            std::bind(&ClangToolRunWorker::onRunnerFinishedWithSuccess, this, runner, _1));
+        onDone(output);
-    connect(runner, &ClangToolRunner::finishedWithFailure, this,
+        handleFinished(runner);
-            std::bind(&ClangToolRunWorker::onRunnerFinishedWithFailure, this, runner, _1, _2));
+    });
     return runner;
 }
 

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();

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) {
+    if (m_process.result() == ProcessResult::FinishedWithSuccess) {
-        emit finishedWithFailure(generalProcessError(m_name), commandlineAndOutput());
-    } else if (m_process.result() == ProcessResult::FinishedWithSuccess) {
         qCDebug(LOG).noquote() << "Output:\n" << m_process.cleanedStdOut();
-        emit finishedWithSuccess(m_input.unit.file);
+        emit done({true, m_input.unit.file, m_outputFilePath, m_name});
-    } else if (m_process.result() == ProcessResult::FinishedWithError) {
+        return;
-        emit finishedWithFailure(finishedWithBadExitCode(m_name, m_process.exitCode()),
-                                 commandlineAndOutput());
-    } else { // == QProcess::CrashExit
-        emit finishedWithFailure(finishedDueToCrash(m_name), commandlineAndOutput());
     }
-}
 
-QString ClangToolRunner::commandlineAndOutput() const
+    const QString details = tr("Command line: %1\n"
-{
+                               "Process Error: %2\n"
-    return tr("Command line: %1\n"
+                               "Output:\n%3")
-              "Process Error: %2\n"
+                                .arg(m_process.commandLine().toUserOutput())
-              "Output:\n%3")
+                                .arg(m_process.error())
-        .arg(m_process.commandLine().toUserOutput())
+                                .arg(m_process.cleanedStdOut());
-        .arg(m_process.error())
+    QString message;
-        .arg(m_process.cleanedStdOut());
+    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

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 done(const AnalyzeOutputData &output);
-    void finishedWithFailure(const QString &errorMessage, const QString &errorDetails);
 
 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;
 };
 

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 {

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;
+    if (!output.success) {
-    FilePath mappedPath = vfso().autoSavedFilePath(m_document);
+        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()),
+        FilePath::fromString(output.outputFilePath),
-        [&](const FilePath &path) { return path == mappedPath; },
+        [&](const FilePath &path) { return path == mappedPath; });
-        &errorMessage);
 
     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) {
+    auto [toDelete, newMarks] = Utils::partition(m_marks, [toolName](DiagnosticMark *mark) {
-        return mark->source == m_currentRunner->name();
+        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,
+    connect(runner, &ClangToolRunner::done, this, &DocumentClangToolRunner::onDone);
-            this, &DocumentClangToolRunner::onSuccess);
-    connect(runner, &ClangToolRunner::finishedWithFailure,
-            this, &DocumentClangToolRunner::onFailure);
     return runner;
 }
 

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 onDone(const AnalyzeOutputData &output);
-    void onFailure(const QString &errorMessage, const QString &errorDetails);
 
     void finalize();