QProcess vs K3Process
Thiago Macieira
thiago at kde.org
Wed Jun 6 18:24:48 BST 2007
Oswald Buddenhagen wrote:
>ok, here is a first implementation of the new kprocess.
Cool!
>kprocess.h
>class KProcessPrivate;
In the .cpp you declare this as struct. Please choose one.
> void setEnv(const QString &name, const QString &value);
No need to shorten it: call this setEnvironment.
> void unsetEnv(const QString &name) { setEnv( name, QString() ); }
Same here, and please uninline this.
> void setProgram(const QString &exe, const QStringList &args);
Not sure if "setProgram" is the best name, but since I can't offer
anything better...
> void setProgram(const QStringList &argv);
I'm sorry, what is this? Shouldn't it be setArguments instead?
> KProcess &operator<<(const QString& arg);
> KProcess &operator<<(const QStringList& args);
I don't like these, but for completeness, also add operator+=.
> * @param shell the path to the shell that will execute the process,
> or * null to use the system's default shell (on *NIX a POSIX shell if
> * available, otherwise any bourne shell, on Windows cmd). * Use
> getenv("SHELL") (*NIX) or getenv("COMSPEC") (Windows) to use * the
> user's default shell, but note that doing so is usually a bad * idea
> for compatibility reasons.
> */
> void setShellCommand(const QString &cmd, const char *shell = 0);
Why is the shell a const char*? I think a QString would be best, like
everything else.
Note that the result of getenv could change, so it should not be stored.
>
> /**
> * Start the process.
> *
> * @see QProcess::start(const QString &, const QStringList &,
> OpenMode) */
> void start();
Please add the OpenMode argument with a default value of
QIODevice::ReadWrite.
> int execute();
Ok, this one needs no OpenMode because, the QIODevice won't open.
You could also add this blurb to the method's apidox:
This method is roughly equivalent to:
<code>
proc.start();
proc.waitForFinished(-1);
</code>
> static int execute(const QStringList &arguments);
Again the same comment as above: what's this overload with just a
QStringList?
> // too bad one cannot "unshadow" overloads
Sure you can:
using QProcess::execute;
>private:
> // hide those
> void setReadChannelMode();
> void readChannelMode();
> void setProcessChannelMode();
> void processChannelMode();
Same:
using QProcess:setReadChannelMode;
using QProcess:readChannelMode;
using QProcess::setProcessChannelMode;
using QProcess::processChannelMode;
> kprocess.cpp
>#ifdef Q_OS_WIN
># include <windows.h>
Uh... no. Move this to kprocess_win.cpp.
>#else
># include <unistd.h>
># include <errno.h>
The Unix-specific parts should be in kprocess_unix.cpp.
>struct KProcessPrivate {
See above about struct/class. You probably want to change it here.
>void KProcessPrivate::writeAll(const QByteArray &buf, int fd)
[snip]
> } else
> off += ret;
Move this function to the _win/_unix.cpp files.
For aesthetic purposes, please add { and } to the else branch above as
well.
> // workaround for QProcess dropping the read buffer on channel
> switch
> int ba = q->bytesAvailable();
> QByteArray buf = q->read(ba);
>
> q->setReadChannel(good);
> writeAll(q->readAll(), fd);
> q->setReadChannel(bad);
>
> // how "efficient" ...
> for (int i = ba; --i >= 0;)
> q->ungetChar(buf[i]);
Has this been reported to TT?
>#define DUMMYENV "_KPROCESS_DUMMY_="
>
>void KProcess::clearEnvironment()
>{
> setEnvironment(QStringList() << DUMMYENV);
>}
Ugly hack... but I understand you.
> for (QStringList::Iterator it = env.begin(); it != env.end();
> ++it) if (*it == DUMMYENV) {
> env.erase(it);
> break;
> }
Equivalent to:
env.removeAll(QLatin1String(DUMMYENV));
> }
> QString fname(name);
> fname.append('=');
> for (QStringList::Iterator it = env.begin(); it != env.end(); ++it)
> if ((*it).startsWith(fname)) {
> if (value.isNull()) {
> env.erase(it);
> goto hit;
Ewww... please use break;
> } else {
> *it = fname.append(value);
> setEnvironment(env);
> return;
> }
> }
> if (!value.isNull())
> env.append(fname.append(value));
> hit:
> if (env.isEmpty())
> env.append(DUMMYENV);
Suggestion:
if (env.isEmpty() && value.isNull())
env.append(QLatin1String(DUMMYENV)));
else if (!value.isNull())
env.append(fname.append(value));
>void KProcess::setProgram(const QStringList &argv)
>{
> d->args = argv;
> d->prog = d->args.takeFirst();
>}
I don't like this. Sounds like a convenience method asking for trouble.
>void KProcess::setShellCommand(const QString &cmd, const char *shell)
>{
> d->prog = QString::fromLatin1(
> shell ?
> shell :
>#ifdef Q_OS_UNIX
>// #ifdef NON_FREE // ... as they ship non-POSIX /bin/sh
># if !defined(__linux__) && !defined(__FreeBSD__) &&
> !defined(__NetBSD__) && !defined(__OpenBSD__) &&
> !defined(__DragonFly__) && !defined(__GNU__)
Q_OS_LINUX, Q_OS_BSD4 (all BSDs, including MacOSX)
> !access("/usr/xpg4/bin/sh", X_OK) ? // Solaris POSIX ...
> "/usr/xpg4/bin/sh" :
> !access("/bin/ksh", X_OK) ? // ... which links here anyway
> "/bin/ksh" :
> !access("/usr/ucb/sh", X_OK) ? // dunno, maybe superfluous?
> "/usr/ucb/sh" :
># endif
> "/bin/sh"
>#else
>// !access("c:\\windows\\cmd.exe", X_OK) ?
>// "c:\\windows\\cmd.exe" :
>// "c:\\command.com";
>// need to determine system dir and system drive properly.
>// consider COMSPEC - for WinDOS it is always command.com-compatible.
> dunno // about WinNT vs. cmd.
># error i am broken. fix me
>#endif
> );
Well, I trust you copied that from existing code, but can't you make it a
bit more... palatable? ifdefs inside a ternary operator inside a call to
QString::fromLatin1... ugh
Also, please move KProcess::setShellCommand to the platform-specific file.
I know, it's small and all, but sounds cleaner to me.
>#ifdef Q_OS_UNIX
> d->args = QStringList() << "-c" << cmd;
>#else
> // XXX this has a good chance of being broken, definitely for
> command.com. // split on whitespace and make a list of that, maybe?
> don't forget // about consecutive spaces in this case.
> d->args = QStringList() << "/c" << cmd;
I think this is correct. Windows merges all arguments into a single
string, so it doesn't matter if you passed one or several.
>int KProcess::execute(const QStringList &arguments)
>{
> QStringList args = arguments;
> QString prog = args.takeFirst();
> return execute(prog, args);
>}
See comment above.
> #include <kprocess.h>
>
>#include <stdio.h>
>
>void testit(KProcess::OutputChannelMode how, const char *what)
>{
> KProcess p;
> p.setShellCommand("echo foo - stdout; echo bar - stderr >&2");
>
> qDebug("mode: %s", what);
> p.setOutputChannelMode(how);
> qDebug("program output:");
> p.execute();
> qDebug("received stdout:");
> fputs(p.readAllStandardOutput(), stdout);
> qDebug("received stderr:");
> fputs(p.readAllStandardError(), stdout);
> qDebug("=============================");
>}
>
>int main()
>{
> testit(KProcess::SeparateChannels, "separate");
> testit(KProcess::ForwardedChannels, "forwarded");
> testit(KProcess::ForwardedStdoutChannel, "forwarded output");
> testit(KProcess::ForwardedStderrChannel, "forwarded error");
> testit(KProcess::MergedChannels, "merged");
>
> return 0;
>}
The test should be done using QtTest. Also, you're using Unix-specific
code, so it should be #ifdef Q_OS_UNIX.
--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
PGP/GPG: 0x6EF45358; fingerprint:
E067 918B B660 DBD1 105C 966C 33F5 F005 6EF4 5358
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20070606/082ad1db/attachment.sig>
More information about the kde-core-devel
mailing list