QProcess vs K3Process

[Thiago Macieira]

Oswald Buddenhagen wrote:
>On Wed, Jun 06, 2007 at 07:24:48PM +0200, Thiago Macieira wrote:
>> Oswald Buddenhagen wrote:
>> >    void setEnv(const QString &name, const QString &value);
>>
>> No need to shorten it: call this setEnvironment.
>
>eh, no. this is not supposed to shadow/overload the QProcess function -
>it does something slightly different (and you don't want to call it
>setEnvironmentVariable, right?).

I don't see why it shouldn't overload the QProcess function.

>also, setenv is ... uh, well ... POSIX. :)

So is mbsnrtowcs. That's no reason to repeat the same naming mistakes of 
POSIX.

>
>> >    void unsetEnv(const QString &name) { setEnv( name, QString() ); }
>>
>> and please uninline this.
>
>i'm not sure i like that. or rather, i'm sure i don't. this is just a
>convenience wrapper for (supposedly) documented behavior of the "proper"
>function. this is not going to change, ever. why would one want to
>uninline it?

Because removing the environment from a QStringList can be done a lot 
faster than the special cases you've come up with. And you could remove 
the special behaviour of handling null QStrings differently from 
empty-but-not-null QStrings.

>> but for completeness, also add operator+=.
>
>i don't see why operator+= should be added - we never had such a thing
>and it would lead to even more inconsistent code.

If it isn't being used, it can't be inconsistent.

>> >    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.
>
>well, this is going to come from ::getenv() or a hard-coded string in
>most cases, so it is not too far off. but well ...

qgetenv is better. And that returns QByteArray.

>> Note that the result of getenv could change, so it should not be
>> stored.
>
>... it's converted to a QString anyway ...
>... so why not do Just Do It (TM) the conventional way. :}

Consistency. All filenames in Qt are QString. Everything else in 
QProcess/KProcess is QString.

>> >    int execute();
>>
>> You could also add this blurb to the method's apidox:
>>   This method is roughly equivalent to:
>>   <code>
>>     proc.start();
>>     proc.waitForFinished(-1);
>>   </code>
>
>well, i could, but what's the point?

To let the user know what the method does. I was scratching my head for a 
minute trying to understand the difference between start and execute, 
besides the return value. I know, you wrote it in the apidox, but first 
skimming through it missed it.

>> >    // too bad one cannot "unshadow" overloads
>>
>> Sure you can:
>>     using QProcess::execute;
>
>you live and learn. :)
>however, some googling indicates that this might not be universally
>suppported yet?

It is for all compilers we support. You can use it.

>this works for hiding, too?

Hiding and unhiding.

>hmm, maybe one should instead inherit from QProcess privately and
>"import" everything explicitly? that's a lot of usings, though.
>
>> >#ifdef Q_OS_WIN
>> ># include <windows.h>
>>
>> Uh... no. Move this to kprocess_win.cpp.
>
>this feels sooooo silly for a few lines ... oh, well.

From what I understand, your work isn't complete. So, it might be extended 
in the future (near future?).

>> >                goto hit;
>>
>> Ewww... please use break;
>
>this is the equivalent construct to python's for:-break-else:. i find a
>short-distance jump way clearer than having to realize that the below
>condition is mutually exclusive to it. i generally think that
>eliminating gotos for the sake of it is a sickness.

goto in C++ is problematic, if it crosses a variable initialisation. 
Please, try to avoid it.

Besides, I do think that the following is more readable:

>> Suggestion:
>>     if (env.isEmpty() && value.isNull())
>>         env.append(QLatin1String(DUMMYENV)));
>>     else if (!value.isNull())
>>         env.append(fname.append(value));


>> >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)
>
>Q_OS_BSD4 would most probably cover solaris 1.x as well.

It doesn't. It matches only MacOS X (Darwin), FreeBSD, DragonflyBSD, 
NetBSD, OpenBSD and BSDi.

>> Well, I trust you copied that from existing code,
>
>... which i wrote ... :=)
>
>> but can't you make it a bit more... palatable? ifdefs inside a ternary
>> operator inside a call to QString::fromLatin1... ugh
>
>see, i *hate* duplication. ;)

I *love* readable code. :-)

We're not participating in the IOCCC.

>> >#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.
>
>the problem is that QProcess will quote the argument so it is
>interpreted as one by "conformant programs". but command.com isn't
>conformant. dunno about cmd.exe, but Win9x doesn't have it anyway.

Well, we'll just have to have one of the Windows guys fix it.

-- 
  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/c6c2827e/attachment.sig>