QProcess vs K3Process

[Thiago Macieira]

Oswald Buddenhagen wrote:
>On Wed, Jun 06, 2007 at 10:52:37PM +0200, Thiago Macieira wrote:
>> 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.
>
>i find it confusing.

That could be true.

But I do think that using a non-terse name is better.

>> >also, setenv is ... uh, well ... POSIX. :)
>>
>> So is mbsnrtowcs.
>
>that's a pretty absurd example. and it is not posix, but gnu (according
>to my man page). :-P

An unhappy example. I knew of the multibyte functions and I just chose one 
that sounded ugly. Of course I ended up with one that is an extension. 
But this one isn't: mbrtowc.

Anyways, the point is: POSIX names are short, terse, compact. That's the 
complete opposite of what we're trying to strive for here. We're looking 
for readability of the code that uses our classes, even if it means more 
lines and more bytes in the .cpp file.

>really, what's wrong with setenv? not in theory, but in practice?

setEnv looks like setEnvironment. Users could be confused about what one 
does and what the other does.

I'd actually prefer this were called something else. There's also putenv 
that you could inspire on.

>
>> >> >    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.
>
>oh?

A simple function that removes the entry from the list is faster than one 
that adds-or-removes entries.

>> And you could remove the special behaviour of handling null QStrings
>> differently from empty-but-not-null QStrings.
>
>no, because that would be an api change. so it is either changed now or
>never => no effect on the inline.

It wouldn't be without precedent that null and empty-but-not-null would be 
treated differently. But it's best to avoid such cases.

>> >> but for completeness, also add operator+=.

>i see the similarity to QStringList, but it's not really the same. it's
>more like a data stream to me.

It's more like a list to me. But, ok, don't add it. The less cruft we add, 
the better.

>> >> >    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.
>
>well, usually one would have a different objective, so such problems
>should not occur. redundancy costs brain bandwitdh, and i relly need all
>of it when reading api docs. :)

Oh, c'mon, just make your documentation better. Don't argue with enhacing 
documentation.

>> 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));
>
>see, i think exactly the opposite. instead of two separate statements we
>now have a compound statement with duplicated conditions. it costs me a
>lot more time to parse this.

You had two separate statements with a label in the middle. Sorry, that's 
less readable to me.

Also, as soon as someone needs to refactor the code -- for whatever 
reason -- there's a great chance that they'll remove the goto. So do us a 
favour and do it already.

>> >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.
>
>hmm, does BSDi have a posix /bin/sh?

I don't know who has a BSDi. I can't confirm.

>> >> 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. :-)
>
>coincidentally, i find my variant more readable than any "expanded"
>version. simply because i don't have to parse the statements to discover
>similarities.

I'm sorry, but you must be quite alone in this.

Ternary operators calling access() inside #ifdef inside ternary operators 
inside a function call. That's what you did.

That's not readable. As I said, it's best to waste a few more lines and a 
few more bytes if this makes the code more readable. You will not be the 
only person reading this code.

>> Well, we'll just have to have one of the Windows guys fix it.
>
>obviously. i'd prefer it if one of them would speak up now, though. oh,
>well, they'll notice when it says "error: ...". :)

It would be best if you called:
  if (shell.isEmpty)
      shell = qgetenv("COMSPEC");
  QProcess::start(QString::fromLatin1("%1 /c %2")
                  .arg(shell, cmd));

when the program were run, instead of trying to split into a QStringList.

-- 
  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/20070607/5d227b0e/attachment.sig>