QProcess vs K3Process

[Oswald Buddenhagen]

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.

> >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
really, what's wrong with setenv? not in theory, but in practice?

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

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

> >> 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.
> 
but then it shouldn't be added.
i see the similarity to QStringList, but it's not really the same. it's
more like a data stream to me.

> >> >    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. :)

> >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?).
> 
well, yes, but we still can split it when it becomes painful. so far,
the pain of splitting it would be way bigger.

> > 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. 
>
but it doesn't here. and gcc is pretty blunt about this violation
anyway.

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

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

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

> We're not participating in the IOCCC.
> 
that would be #ifdef-ing braces at different indentation levels and
similar "fun". :=)

> 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: ...". :)

-- 
Hi! I'm a .signature virus! Copy me into your ~/.signature, please!
--
Chaos, panic, and disorder - my work here is done.