KProcess Line Mode Patch

[Ralf Habacker]

Thiago Macieira schrieb:
> On Monday 01 December 2008 12:32:08 Ralf Habacker wrote:
>   
>> Hi,
>>
>> based on the work described on
>> http://lists.kde.org/?l=kde-core-devel&m=120522889930459&w=2 there is a
>> patch appended which implements a line buffering mode directly in KProcess.
>> The advantage of this approach is that this feature is directly
>> available from kdecore and therefore no additional class located
>> somewhere in the related package source is required.
>>
>>  How it works:
>>
>> 1.Line buffering mode ist enabled with the
>>
>>     void setOutputLineMode(bool mode);
>>
>> 2.There are new KProcess signals available which indicates available lines.
>>
>>     void readyReadLineStandardOutput();
>>     void readyReadLineStandardError();
>>
>> 3. The lines could be fetched with
>>
>>     QStringList &readLinesStandardOutput(QStringList &out=QStringList());
>>     QStringList &readLinesStandardError(QStringList &out=QStringList());
>>
>> 4. non standard line delimiters could be set by
>>
>>     void setOutputLineDelimiter(QString &delim=QString());
>>
>> Any comments or objections ?
>>     
>
> Yes, I object :-)
>
> A few problems with your approach:
>
> 1) you're using QByteArray for storing the buffer. That's incredibly 
> inefficient for large buffers. Imagine that the child program is writing 1MB 
> lines: that will cause a lot of memory reallocations and memcpy.
>
> Please use peek() from QIODevice to determine if there's a newline in the 
> buffer. You could/should also restrict the length of the search for newline. 
> By doing this, you let QIODevice / QProcess manage the buffer, which it does 
> using the more-efficient QRingBuffer class (not public)
>
> If you choose not to do this, then I recommend you only read the first line. 
> There's no need to keep reading in _k_receivedStdout and _k_receivedStderr if 
> you already have one line read.
>
> 2) Note that, with your patch, then you *cannot* use the normal 
> QProcess/KProcess functions read(), readLine(), peek(), etc. because you 
> drained the buffers. KProcess does not override the QProcess virtuals 
> readData() and writeData(), meaning those still operate on QProcess buffers.
>
> In fact, with your patch, you're restricted to the new line mode. The non-line 
> mode will not work.
>
> 3) The API you presented:
>
>     QStringList &readLinesStandardOutput(QStringList &out=QStringList());
>     QStringList &readLinesStandardError(QStringList &out=QStringList());
>
> is IMHO awful. It has no parallel in Qt or KDE core classes. It violates the 
> Library Coding Policy by returning a reference. And it takes a reference as a 
> parameter, which has a default value(!). Worse yet, you return that temporary 
> as a non-const reference. 
>
> I think the compiler doesn't even let you write:
> 	QStringList &out = QStringList();
>
> See:
> $ echo 'class Foo {}; int main() { Foo &foo = Foo(); }' | \
> pipe> /usr/bin/g++ -fsyntax-only -xc++ -
> <stdin>: In function ‘int main()’:
> <stdin>:1: error: invalid initialization of non-const reference of type ‘Foo&’ 
> from a temporary of type ‘Foo’
>
> Please use QString returns and read one line at a time. That will parallel the 
> readLine() function that is already there, by extending it to match what the 
> readAll()/readAllStandardError()/readAllStandardOutput() do. That would 
> require you to have a canReadLineStandardError and canReadLineStandardOutput() 
> already.
>
> 4) Your signals emit only once. The readyRead(), readyReadStandardOutput() and  
> and readyReadStandardError() signals emit whenever there's new data. 
>
> If you choose to continue with this, your code should emit the signals 
> whenever there's a new newline. But I don't see a way of efficiently 
> implementing this without direct access to QProcess's buffers -- which 
> QProcess won't give you.
>
> 5) Finally, I'm not convinced of the need for this. Why can't people call 
> canReadLine() when readyRead() is emitted?
>   
I see, it makes no sense to continue this way, so I reject this patch.

> What I'd recommend instead is a line-based reader that works on top of a plain 
> QIODevice. That way, you can plug it on a QTcpSocket, QLocalSocket, 
> QNetworkReply or KTcpSocket objects as well. (the finished() signal is 
> replaced by the readChannelFinished() signal)
>   
> This would have the issue, of course, that it doesn't handle a multi-channel 
> device like QProcess.
>
>   
Appended is a proposal for such a class working with the 
QProcess/KProcess output channel.

One question I have is where in the kdelibs such a class could be located ?

Ralf

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: klinebasedreader.patch
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20081202/536319c1/attachment.ksh>