feedc0de/qt-creator
1
0
Fork 0
forked from qt-creator/qt-creator
Code Pull Requests Activity

QtcProcess: Make the class final

Browse Source 
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 <hjk@qt.io>
This commit is contained in:
Jarek Kobus
2022-05-06 11:59:17 +02:00
parent 2856091c75
commit 6455d7fcf3
4 changed files with 21 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
src/libs/utils/qtcprocess.cpp
View File

@@ -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)

9
src/libs/utils/qtcprocess.h
View File

@@ -30,7 +30,6 @@
#include "environment.h"
#include "commandline.h"
#include "processenums.h"
#include "qtcassert.h"
#include <QProcess>
@@ -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);

1
src/plugins/docker/dockerapi.cpp
View File

@@ -26,6 +26,7 @@
#include "dockerapi.h"
#include <coreplugin/progressmanager/progressmanager.h>
#include <utils/qtcassert.h>
#include <utils/qtcprocess.h>
#include <utils/runextensions.h>

51
tests/auto/utils/qtcprocess/tst_qtcprocess.cpp
View File

@@ -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<AbstractMacroExpander*> &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));