From 6455d7fcf34007ef3d2e32bc004ac2cf7220e33e Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Fri, 6 May 2022 11:59:17 +0200 Subject: [PATCH] QtcProcess: Make the class final Currently, as a result of many refactorings, the QtcProcess is designed so that if you want to customize the behavior you should implement custom ProcessInterface subclass. Adding virtual methods directly to QtcProcess caused tons of unpredicted side effects and nasty bugs, which were hard to track and hard to fix, as provided fixes were usually fixing particular case while introducing regressions in not related code paths. Consider also aggregating QtcProcess object instead of deriving from it when some additional methods are required. This patch removes the last virtual methods from QtcProcess API and makes the class final in order to prevent from adding any new virtual methods to this class in the future. This commit message should make it clear that having subclasses of QtcProcess is not a desired design. It's a post-mortem conclusion. So: don't derive from QtcProcess - we mean it! Change-Id: I1e43ed45be326b366422fd7db6e05ba48ea5fb98 Reviewed-by: hjk --- src/libs/utils/qtcprocess.cpp | 23 ++------- src/libs/utils/qtcprocess.h | 9 +--- src/plugins/docker/dockerapi.cpp | 1 + .../auto/utils/qtcprocess/tst_qtcprocess.cpp | 51 ++++++------------- 4 files changed, 21 insertions(+), 63 deletions(-) diff --git a/src/libs/utils/qtcprocess.cpp b/src/libs/utils/qtcprocess.cpp index 309294403e4..5ab723b6a8d 100644 --- a/src/libs/utils/qtcprocess.cpp +++ b/src/libs/utils/qtcprocess.cpp @@ -989,16 +989,6 @@ QtcProcess::~QtcProcess() delete d; } -void QtcProcess::emitStarted() -{ - emit started(); -} - -void QtcProcess::emitFinished() -{ - emit finished(); -} - void QtcProcess::setProcessImpl(ProcessImpl processImpl) { d->m_setup.m_processImpl = processImpl; @@ -1099,19 +1089,14 @@ void QtcProcess::start() QTC_ASSERT(state() == QProcess::NotRunning, return); d->clearForRun(); d->m_state = QProcess::Starting; - startImpl(); -} - -void QtcProcess::startImpl() -{ ProcessInterface *processImpl = nullptr; if (d->m_setup.m_commandLine.executable().needsDevice()) { - QTC_ASSERT(s_deviceHooks.processImplHook, return); + QTC_ASSERT(s_deviceHooks.processImplHook, d->m_state = QProcess::NotRunning; return); processImpl = s_deviceHooks.processImplHook(commandLine().executable()); } else { processImpl = d->createProcessInterface(); } - QTC_ASSERT(processImpl, return); + QTC_ASSERT(processImpl, d->m_state = QProcess::NotRunning; return); d->setProcessInterface(processImpl); d->m_process->m_setup = d->m_setup; d->m_process->m_setup.m_commandLine = d->fullCommandLine(); @@ -2001,13 +1986,13 @@ void QtcProcessPrivate::handleError() void QtcProcessPrivate::emitStarted() { CALL_STACK_GUARD(); - q->emitStarted(); + emit q->started(); } void QtcProcessPrivate::emitFinished() { CALL_STACK_GUARD(); - q->emitFinished(); + emit q->finished(); } void QtcProcessPrivate::emitErrorOccurred(QProcess::ProcessError error) diff --git a/src/libs/utils/qtcprocess.h b/src/libs/utils/qtcprocess.h index 54236bf27be..8da597767c4 100644 --- a/src/libs/utils/qtcprocess.h +++ b/src/libs/utils/qtcprocess.h @@ -30,7 +30,6 @@ #include "environment.h" #include "commandline.h" #include "processenums.h" -#include "qtcassert.h" #include @@ -49,7 +48,7 @@ class DeviceProcessHooks; class ProcessInterface; class ProcessResultData; -class QTCREATOR_UTILS_EXPORT QtcProcess : public QObject +class QTCREATOR_UTILS_EXPORT QtcProcess final : public QObject { Q_OBJECT @@ -202,12 +201,6 @@ signals: void readyReadStandardOutput(); void readyReadStandardError(); -protected: - // TODO: remove these methods on QtcProcess de-virtualization - virtual void startImpl(); - virtual void emitStarted(); - virtual void emitFinished(); - private: friend QTCREATOR_UTILS_EXPORT QDebug operator<<(QDebug str, const QtcProcess &r); diff --git a/src/plugins/docker/dockerapi.cpp b/src/plugins/docker/dockerapi.cpp index 7c57cf06187..77260ed158e 100644 --- a/src/plugins/docker/dockerapi.cpp +++ b/src/plugins/docker/dockerapi.cpp @@ -26,6 +26,7 @@ #include "dockerapi.h" #include +#include #include #include diff --git a/tests/auto/utils/qtcprocess/tst_qtcprocess.cpp b/tests/auto/utils/qtcprocess/tst_qtcprocess.cpp index 778c1f5a848..ec8bf7454f1 100644 --- a/tests/auto/utils/qtcprocess/tst_qtcprocess.cpp +++ b/tests/auto/utils/qtcprocess/tst_qtcprocess.cpp @@ -84,27 +84,6 @@ protected: int MessageHandler::s_destroyCount = 0; QtMessageHandler MessageHandler::s_oldMessageHandler = 0; -static std::atomic_int s_processCounter = 0; -static const bool s_removeSingletonsOnLastProcess = false; -// The use of this class (when s_removeSingletonsOnLastProcess = true) guarantees that if all -// instances of QtcProcess are destructed at some point (i.e. after each test function ends) -// we also run process reaper's and process launcher's destructors. Otherwise -// (when s_removeSingletonsOnLastProcess = true) this is a no-op wrapper around QtcProcess. -class TestProcess : public QtcProcess -{ -public: - class TestProcessDeleter : public QObject - { - public: - TestProcessDeleter(QObject *parent) : QObject(parent) { s_processCounter.fetch_add(1); } - ~TestProcessDeleter() { - if ((s_processCounter.fetch_sub(1) == 1) && s_removeSingletonsOnLastProcess) - Utils::Singleton::deleteAll(); - } - }; - TestProcess() { new TestProcessDeleter(this); } -}; - class MacroMapExpander : public AbstractMacroExpander { public: virtual bool resolveMacro(const QString &name, QString *ret, QSet &seen) @@ -878,7 +857,7 @@ void tst_QtcProcess::exitCode() SubProcessConfig subConfig(ProcessTestApp::ExitCode::envVar(), QString::number(exitCode)); { - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.start(); const bool finished = process.waitForFinished(); @@ -888,7 +867,7 @@ void tst_QtcProcess::exitCode() QCOMPARE(process.exitCode() == 0, process.result() == ProcessResult::FinishedWithSuccess); } { - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.runBlocking(); @@ -925,7 +904,7 @@ void tst_QtcProcess::runBlockingStdOut() QFETCH(ProcessResult, expectedResult); SubProcessConfig subConfig(ProcessTestApp::RunBlockingStdOut::envVar(), withEndl ? "true" : "false"); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.setTimeoutS(timeOutS); @@ -947,7 +926,7 @@ void tst_QtcProcess::runBlockingStdOut() void tst_QtcProcess::lineCallback() { SubProcessConfig subConfig(ProcessTestApp::LineCallback::envVar(), {}); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); QStringList lines = QString(s_lineCallbackData).split('|'); @@ -970,7 +949,7 @@ void tst_QtcProcess::lineCallback() void tst_QtcProcess::waitForStartedAfterStarted() { SubProcessConfig subConfig(ProcessTestApp::SimpleTest::envVar(), {}); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); bool started = false; @@ -1011,7 +990,7 @@ void tst_QtcProcess::waitForStartedAfterStarted2() void tst_QtcProcess::waitForStartedAndFinished() { SubProcessConfig subConfig(ProcessTestApp::SimpleTest::envVar(), {}); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.start(); @@ -1024,7 +1003,7 @@ void tst_QtcProcess::waitForStartedAndFinished() void tst_QtcProcess::notRunningAfterStartingNonExistingProgram() { - TestProcess process; + QtcProcess process; process.setCommand({ FilePath::fromString( "there_is_a_big_chance_that_executable_with_that_name_does_not_exists"), {} }); @@ -1086,7 +1065,7 @@ void tst_QtcProcess::channelForwarding() SubProcessConfig subConfig(ProcessTestApp::ChannelForwarding::envVar(), QString::number(int(channelMode))); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.start(); @@ -1129,7 +1108,7 @@ void tst_QtcProcess::mergedChannels() QFETCH(bool, errorOnError); SubProcessConfig subConfig(ProcessTestApp::StandardOutputAndErrorWriter::envVar(), {}); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.setProcessChannelMode(channelMode); @@ -1162,7 +1141,7 @@ void tst_QtcProcess::killBlockingProcess() SubProcessConfig subConfig(ProcessTestApp::KillBlockingProcess::envVar(), QString::number(int(blockType))); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.start(); QVERIFY(process.waitForStarted()); @@ -1172,7 +1151,7 @@ void tst_QtcProcess::killBlockingProcess() void tst_QtcProcess::flushFinishedWhileWaitingForReadyRead() { SubProcessConfig subConfig(ProcessTestApp::SimpleTest::envVar(), {}); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.start(); @@ -1197,7 +1176,7 @@ void tst_QtcProcess::flushFinishedWhileWaitingForReadyRead() void tst_QtcProcess::emitOneErrorOnCrash() { SubProcessConfig subConfig(ProcessTestApp::EmitOneErrorOnCrash::envVar(), {}); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); int errorCount = 0; @@ -1216,7 +1195,7 @@ void tst_QtcProcess::emitOneErrorOnCrash() void tst_QtcProcess::crashAfterOneSecond() { SubProcessConfig subConfig(ProcessTestApp::CrashAfterOneSecond::envVar(), {}); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.start(); @@ -1236,7 +1215,7 @@ void tst_QtcProcess::recursiveCrashingProcess() const int recursionDepth = 5; // must be at least 2 SubProcessConfig subConfig(ProcessTestApp::RecursiveCrashingProcess::envVar(), QString::number(recursionDepth)); - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.start(); QVERIFY(process.waitForStarted(1000)); @@ -1268,7 +1247,7 @@ void tst_QtcProcess::recursiveBlockingProcess() SubProcessConfig subConfig(ProcessTestApp::RecursiveBlockingProcess::envVar(), QString::number(recursionDepth)); { - TestProcess process; + QtcProcess process; subConfig.setupSubProcess(&process); process.start(); QVERIFY(process.waitForStarted(1000));