Re: [Development] Enhancement to QIODevice?

[Andrzej Ostruszka]

Thiago, my apologies for replying that late.
For some strange reason gmail has decided to mark your reply as spam
(even though previous conversation was perfectly OK).  Since I see you
active in the mailing list I had to dig deeper to find it.

On Wed, Sep 16, 2015 at 8:17 PM, Thiago Macieira
 wrote:
[...]
> Well, yeah, it needs to read at least one from the lower device into the
> QIODevice buffer. It will then copy the data again to your buffer. There's no
> way to avoid the double copy, unless device implementation (QSerialPort?)
> already read into the QIODevice buffer. Please talk to Alex Trotsenko to
> optimise this.
>
> But canReadLine() needs to do something similar already. If the data isn't
> buffered yet, it needs to call readData() to copy into the buffer first, then
> read it.

This is why I talked about peek() from the buffer itself since I
wanted to avoid reading from device.
My wish was to handle QSerialPort asynchronously and just peek in the
buffer (and if there is not enough data then just wait for more).

>> C. Add QIODPLB::data() function to buffer which will return "first" (I'm
>> talking in terms of QIODPLB implementation)
>
> Returning access to the internal buffer is something to consider. It would
> allow for one memcpy fewer of the data. But it's not something that we can
> trivially do.
>
[...]
>
> The internal buffer is chunked, so the delimiter may not be in the first 
> buffer.
> And I don't want to provide access to the internal buffers without a lot of
> thought to design, since after that it becomes ABI and we will have less
> freedom to improve.

Is it really chunked?  As I've written previously I'm not wearing
"expert" hat here but QIODevicePrivateLinearBuffer seems to be a nice
and simple linear buffer (with data remaining after read() being moved
to the beginning of buffer).  This buffer is then used in
QIODevicePrivate which is then used in QIODevice.

And in case of QSerialPort on unix reading goes like:
1. Notification from QSocketNotifier ready to read is passed to
QSerialPortPrivate::readNotification()
2. There, buffer.reserve() returns pointer to write to and we read
from port to it

Similar thing is done in case of windows implementation - we schedule
asynchronous read to some internal "readChunkBuffer" and in completion
of this operation we append what was read to the
QIODevicePrivateLinearBuffer.

>From the user perspective this difference is hidden - he gets
readyRead() notification and then it has all read data available in
buffer so I don't see chunking here in the internals - do I miss
anything?

Maybe you are thinking about chunking during reading (in case of
QSerialPort we are reading in 512 chunks but the data already
buffered/available are just one linear array).

If my understanding is correct then I see no problem in adding simple:

const char* QIODevicePrivateLinearBuffer::data() const { return first; }

> QIODevice needs a complete redesign to support zero-copy. The current API
> created in Qt 4 (readData(), writeData(), etc.) is way too limited. The
> problem is that there's no way to do that without a major break in
> compatibility (so, Qt 6) and spending a *lot* of time fixing all the QIODevice
> subclasses (QBuffer, QFileDevice, QAbstractSocket, QProcess, QNetworkReply,
> QSerialPort, plus all the non-Qt code).

I don't want to suggest that significant change.  I mean I don't
object any such attempt but I think I'd be satisfied with this little
addition above.

[...]
> I understand the value of countTo(), but it can spiral out of control really
> quickly. After we add the one containing a single byte delimiter, we'll get
> people asking for:
>  - string searching (like CRLF)
>  - regular expression
>  - state machine searching (CSV: comma, unless inside a "")

That's why I prefer now just opening internal buffer implementation to
allow for read-only access to data already buffered instead of my
original countTo() suggestion.  Any such custom request can be built
by the user by subclassing QSerialPort/... and accessing data
available in buffer.

Best regards
Andrzej
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Thiago Macieira]

On Monday 21 September 2015 12:32:36 Andrzej Ostruszka wrote:
> > Well, yeah, it needs to read at least one from the lower device into the
> > QIODevice buffer. It will then copy the data again to your buffer. There's
> > no way to avoid the double copy, unless device implementation
> > (QSerialPort?) already read into the QIODevice buffer. Please talk to
> > Alex Trotsenko to optimise this.
> > 
> > But canReadLine() needs to do something similar already. If the data isn't
> > buffered yet, it needs to call readData() to copy into the buffer first,
> > then read it.
> 
> This is why I talked about peek() from the buffer itself since I
> wanted to avoid reading from device.

We're talking about two different things here. I was talking about read() and 
peek(), which functions need to readData() from the device in order to do any 
scanning of any kind. Your suggestion will not change this.

You're talking about the user's point of view.

> >> C. Add QIODPLB::data() function to buffer which will return "first" (I'm
> >> talking in terms of QIODPLB implementation)
> > 
> > Returning access to the internal buffer is something to consider. It would
> > allow for one memcpy fewer of the data. But it's not something that we can
> > trivially do.
> 
> [...]
> 
> > The internal buffer is chunked, so the delimiter may not be in the first
> > buffer. And I don't want to provide access to the internal buffers
> > without a lot of thought to design, since after that it becomes ABI and
> > we will have less freedom to improve.
> 
> Is it really chunked?  As I've written previously I'm not wearing
> "expert" hat here but QIODevicePrivateLinearBuffer seems to be a nice
> and simple linear buffer (with data remaining after read() being moved
> to the beginning of buffer).  This buffer is then used in
> QIODevicePrivate which is then used in QIODevice.

You're right about QIODevicePrivate, but that's not the buffer used in QProcess 
and QAbstractSocket. With Alex Trotsenko's work, we could change all of them 
to QRingBuffer.

The point is: I want to be able to assume it's chunked. Therefore, returning 
the buffer is a no-no until we think a lot about it.

> And in case of QSerialPort on unix reading goes like:
> 1. Notification from QSocketNotifier ready to read is passed to
> QSerialPortPrivate::readNotification()
> 2. There, buffer.reserve() returns pointer to write to and we read
> from port to it

Nothing there says it cannot be chunked. You've just described QAbstractSocket 
too, but that one is chunked.

> Similar thing is done in case of windows implementation - we schedule
> asynchronous read to some internal "readChunkBuffer" and in completion
> of this operation we append what was read to the
> QIODevicePrivateLinearBuffer.

And this one sounds like QProcess, except that QWindowsPipeReader is an order 
of magnitude more complex.

> If my understanding is correct then I see no problem in adding simple:
> 
> const char* QIODevicePrivateLinearBuffer::data() const { return first; }

It's not that simple.

Please replace QIODevicePrivateLinearBuffer with QRingBuffer and send your 
reply 
again.

> > QIODevice needs a complete redesign to support zero-copy. The current API
> > created in Qt 4 (readData(), writeData(), etc.) is way too limited. The
> > problem is that there's no way to do that without a major break in
> > compatibility (so, Qt 6) and spending a *lot* of time fixing all the
> > QIODevice subclasses (QBuffer, QFileDevice, QAbstractSocket, QProcess,
> > QNetworkReply, QSerialPort, plus all the non-Qt code).
> 
> I don't want to suggest that significant change.  I mean I don't
> object any such attempt but I think I'd be satisfied with this little
> addition above.

Yeah, but your little addition requires the thinking ahead that I am talking 
about.

> > I understand the value of countTo(), but it can spiral out of control
> > really quickly. After we add the one containing a single byte delimiter,
> > we'll get> 
> > people asking for:
> >  - string searching (like CRLF)
> >  - regular expression
> >  - state machine searching (CSV: comma, unless inside a "")
> 
> That's why I prefer now just opening internal buffer implementation to
> allow for read-only access to data already buffered instead of my
> original countTo() suggestion.  Any such custom request can be built
> by the user by subclassing QSerialPort/... and accessing data
> available in buffer.

And I'd like buffer access, but I am not going to allow it until we think 
raelly well about future compatibility.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Thiago Macieira]

On Tuesday 15 September 2015 11:41:10 Andrzej Ostruszka wrote:
> On 09/15/2015 03:53 AM, Thiago Macieira wrote:
> > On Monday 14 September 2015 12:25:20 Andrzej Ostruszka wrote:
> >> So what I'm aiming to is to make the "line" (or more generally "packet")
> >> termination a bit more flexible, that is to allow user to specify the
> >> delimiter.  Currently I think the only available solution for my wish is
> >> double buffering - that is to introduce my own buffer that will suck
> >> everything from QIODevice and allow searching for other delimiter but
> >> I'd like to do better than that.
> > 
> > You could use peek() to search the buffer, then read() exactly as much as
> > you really need.
> 
> I understand that we are talking about
> QIODevicePrivateLinearBuffer::peek() here.

No, I meant QIODevice::peek()

> In your approach I'd be copying data twice, but I can eliminate one with
> peek()ing
> directly to the output buffer and later skip()ping in successful case.
> However in case
> when delimiter is not yet available I'd have unnecessary copying.

Then please provide a patch that avoids the unnecessary second copy when the 
read() destination is a null pointer.

> And while peek()ing I'd have to use the min(max size of my output
> buffer, size())
> which would mean that I'd be copying "overlapping" parts more than once.

I didn't understand this.

> This is suboptimal.
> 
> Should I drop this proposal?  From other replies I gather that I'm not
> the only one
> that would welcome this or similar enhancement.

I think there are easier ways to do this, so I don't think it makes sense to 
accept this suggestion.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Andrzej Ostruszka]

On 09/16/2015 08:57 AM, Thiago Macieira wrote:
>>> You could use peek() to search the buffer, then read() exactly as much as
>>> you really need.
>>
>> I understand that we are talking about
>> QIODevicePrivateLinearBuffer::peek() here.
>
> No, I meant QIODevice::peek()

First of all - I do not claim that I have mastered Qt internals!
So everything below you should take with a grain of salt :)

QIODevice::peek() is implemented in terms of QIODevice::read() (which 
then uses QIODevicePrivate::read() which does the memcpy()) and later 
putting it back.  Pretty convoluted but it really does not matter - I 
just assumed the simpler case of direct peek into the buffer.

>> In your approach I'd be copying data twice, but I can eliminate one with
>> peek()ing
>> directly to the output buffer and later skip()ping in successful case.
>> However in case
>> when delimiter is not yet available I'd have unnecessary copying.
>
> Then please provide a patch that avoids the unnecessary second copy when the
> read() destination is a null pointer.

Here I must say that I don't understand.  Let me rephrase what I've said 
earlier.  Suppose that there is some data in buffer and I want to know 
if delimiter is in it.  Without direct access to this buffer memory I 
can only:
1. copy (by peeking()) it to some other place (possibly the final output 
buffer)
2. search for delimiter in my copy
3. if found: drop "number of bytes to delimiter" from buffer

What I'm saying is that in successful case (when delimiter is found) 
this sequence is optimal.  However, in case when there is no delimiter 
yet in the buffer, operation 1 is wasted (not really needed).  And I 
don't see how to avoid it without allowing user to search in the 
internal buffer.

Possible ways to allow for searching in this buffer are (I'm shortening 
names here):
A. Add optional param to canReadLine() readLine() (for QIODP or maybe 
even QIOD)
B. Add this QIODPLB::countTo() functionality (which does not break or 
interfere with anything)
C. Add QIODPLB::data() function to buffer which will return "first" (I'm 
talking in terms of QIODPLB implementation)
D. ???

As for C.  This is another solution - which also does not interfere with 
anything.  The amount of data is already available via size().
Then any kind of "packet" termination can be implemented (not just by 
single char) on top of it.  Value returned by this QIODPLB::data() is 
only invalidated by explicit user reading so this would be pretty safe 
too and its only one line of code.

So what do you think?

>> And while peek()ing I'd have to use the min(max size of my output
>> buffer, size())
>> which would mean that I'd be copying "overlapping" parts more than once.
>
> I didn't understand this.

What I wanted to say is that:
- in operation 1 above the number of bytes to peek need to be specified
- the only reasonable choice - without using some application specific 
heuristic - is the amount of bytes already available (lets forget for a 
moment possible constrains of my output buffer size)

Then in situation when in buffer is more then just one "line" available 
I would be copying parts of data after first delimiter in order to later 
put them back and read them again on next data arrival.

> I think there are easier ways to do this, so I don't think it makes sense to
> accept this suggestion.

Yes one can peek() at buffer but as I've tried to state this approach 
has some drawbacks (from performance point of view).  I can also 
understand that maintainers are worried about growing complexity and 
maintenance burden but honestly I don't see any growth in these from 
options B and C above.  These are internal, no testing for them is 
needed, they don't interfere with any other functionality etc.

So Your Honour, what is the verdict? ;)

Regards
Andrzej
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Mark Gaiser]

On Wed, Sep 16, 2015 at 1:13 PM, Andrzej Ostruszka <
andrzej.ostrus...@gmail.com> wrote:

> On 09/16/2015 08:57 AM, Thiago Macieira wrote:
> >>> You could use peek() to search the buffer, then read() exactly as much
> as
> >>> you really need.
> >>
> >> I understand that we are talking about
> >> QIODevicePrivateLinearBuffer::peek() here.
> >
> > No, I meant QIODevice::peek()
>
> First of all - I do not claim that I have mastered Qt internals!
> So everything below you should take with a grain of salt :)
>
> QIODevice::peek() is implemented in terms of QIODevice::read() (which
> then uses QIODevicePrivate::read() which does the memcpy()) and later
> putting it back.  Pretty convoluted but it really does not matter - I
> just assumed the simpler case of direct peek into the buffer.
>
> >> In your approach I'd be copying data twice, but I can eliminate one with
> >> peek()ing
> >> directly to the output buffer and later skip()ping in successful case.
> >> However in case
> >> when delimiter is not yet available I'd have unnecessary copying.
> >
> > Then please provide a patch that avoids the unnecessary second copy when
> the
> > read() destination is a null pointer.
>
> Here I must say that I don't understand.  Let me rephrase what I've said
> earlier.  Suppose that there is some data in buffer and I want to know
> if delimiter is in it.  Without direct access to this buffer memory I
> can only:
> 1. copy (by peeking()) it to some other place (possibly the final output
> buffer)
> 2. search for delimiter in my copy
> 3. if found: drop "number of bytes to delimiter" from buffer
>
> What I'm saying is that in successful case (when delimiter is found)
> this sequence is optimal.  However, in case when there is no delimiter
> yet in the buffer, operation 1 is wasted (not really needed).  And I
> don't see how to avoid it without allowing user to search in the
> internal buffer.
>
> Possible ways to allow for searching in this buffer are (I'm shortening
> names here):
> A. Add optional param to canReadLine() readLine() (for QIODP or maybe
> even QIOD)
> B. Add this QIODPLB::countTo() functionality (which does not break or
> interfere with anything)
> C. Add QIODPLB::data() function to buffer which will return "first" (I'm
> talking in terms of QIODPLB implementation)
> D. ???
>
> As for C.  This is another solution - which also does not interfere with
> anything.  The amount of data is already available via size().
> Then any kind of "packet" termination can be implemented (not just by
> single char) on top of it.  Value returned by this QIODPLB::data() is
> only invalidated by explicit user reading so this would be pretty safe
> too and its only one line of code.
>
> So what do you think?
>
> >> And while peek()ing I'd have to use the min(max size of my output
> >> buffer, size())
> >> which would mean that I'd be copying "overlapping" parts more than once.
> >
> > I didn't understand this.
>
> What I wanted to say is that:
> - in operation 1 above the number of bytes to peek need to be specified
> - the only reasonable choice - without using some application specific
> heuristic - is the amount of bytes already available (lets forget for a
> moment possible constrains of my output buffer size)
>
> Then in situation when in buffer is more then just one "line" available
> I would be copying parts of data after first delimiter in order to later
> put them back and read them again on next data arrival.
>
> > I think there are easier ways to do this, so I don't think it makes
> sense to
> > accept this suggestion.
>
> Yes one can peek() at buffer but as I've tried to state this approach
> has some drawbacks (from performance point of view).  I can also
> understand that maintainers are worried about growing complexity and
> maintenance burden but honestly I don't see any growth in these from
> options B and C above.  These are internal, no testing for them is
> needed, they don't interfere with any other functionality etc.
>
> So Your Honour, what is the verdict? ;)
>
> Regards
> Andrzej
>

I'm curious, why is QIODevicePrivate::peek (both versions of it) doing:
- calling read (which resizes the internal buffer to whatever is left after
reading). It does this via memmove.
- then it restores the buffer to it's original by taking the current buffer
and appending what was read. It does this via memcpy.

Rather then doing:
- call QIODevicePrivateLinearBuffer::peek (on the buffer object in
QIODevicePrivate) and leave the internal buffer untouched? The function is
just sitting there [1], but it isn't being used by either
QIODevice(Private)::peek function.

It would avoid fiddling with the internal buffer and just copy (a part of)
the internal buffer to the user. That is what peek should do according to
the documentation.
Peek would the not call QIODevice::read anymore.

Performance wise, this likely makes peek faster (no more read call, no more
memmove, no more memcpy on the internal buffer), memory wise there still is

Re: [Development] Enhancement to QIODevice?

[Thiago Macieira]

On Wednesday 16 September 2015 13:13:01 Andrzej Ostruszka wrote:
> On 09/16/2015 08:57 AM, Thiago Macieira wrote:
> >>> You could use peek() to search the buffer, then read() exactly as much
> >>> as
> >>> you really need.
> >> 
> >> I understand that we are talking about
> >> QIODevicePrivateLinearBuffer::peek() here.
> > 
> > No, I meant QIODevice::peek()
> 
> First of all - I do not claim that I have mastered Qt internals!
> So everything below you should take with a grain of salt :)

No problem. I don't remember the internals of peek(), but I do remember it 
trying to unread each character, one by one. Or was that 
QProcess::setReadChannel...?

> QIODevice::peek() is implemented in terms of QIODevice::read() (which
> then uses QIODevicePrivate::read() which does the memcpy()) and later
> putting it back.  Pretty convoluted but it really does not matter - I
> just assumed the simpler case of direct peek into the buffer.

Well, yeah, it needs to read at least one from the lower device into the 
QIODevice buffer. It will then copy the data again to your buffer. There's no 
way to avoid the double copy, unless device implementation (QSerialPort?) 
already read into the QIODevice buffer. Please talk to Alex Trotsenko to 
optimise this.

But canReadLine() needs to do something similar already. If the data isn't 
buffered yet, it needs to call readData() to copy into the buffer first, then 
read it.

> Here I must say that I don't understand.  Let me rephrase what I've said
> earlier.  Suppose that there is some data in buffer and I want to know
> if delimiter is in it.  Without direct access to this buffer memory I
> can only:
> 1. copy (by peeking()) it to some other place (possibly the final output
> buffer)
> 2. search for delimiter in my copy
> 3. if found: drop "number of bytes to delimiter" from buffer

Right.

Note that this is already an improvement over status quo: right now, you need 
to read() everything and buffer the data that you didn't need just yet.

> What I'm saying is that in successful case (when delimiter is found)
> this sequence is optimal.  However, in case when there is no delimiter
> yet in the buffer, operation 1 is wasted (not really needed).  And I
> don't see how to avoid it without allowing user to search in the
> internal buffer.

I see.

> Possible ways to allow for searching in this buffer are (I'm shortening
> names here):
> A. Add optional param to canReadLine() readLine() (for QIODP or maybe
> even QIOD)

Not to those functions. It would be a new set of them.

> B. Add this QIODPLB::countTo() functionality (which does not break or
> interfere with anything)
> C. Add QIODPLB::data() function to buffer which will return "first" (I'm
> talking in terms of QIODPLB implementation)

Returning access to the internal buffer is something to consider. It would 
allow for one memcpy fewer of the data. But it's not something that we can 
trivially do.

> As for C.  This is another solution - which also does not interfere with
> anything.  The amount of data is already available via size().
> Then any kind of "packet" termination can be implemented (not just by
> single char) on top of it.  Value returned by this QIODPLB::data() is
> only invalidated by explicit user reading so this would be pretty safe
> too and its only one line of code.
> 
> So what do you think?

The internal buffer is chunked, so the delimiter may not be in the first 
buffer. 
And I don't want to provide access to the internal buffers without a lot of 
thought to design, since after that it becomes ABI and we will have less 
freedom to improve.

QIODevice needs a complete redesign to support zero-copy. The current API 
created in Qt 4 (readData(), writeData(), etc.) is way too limited. The 
problem is that there's no way to do that without a major break in 
compatibility (so, Qt 6) and spending a *lot* of time fixing all the QIODevice 
subclasses (QBuffer, QFileDevice, QAbstractSocket, QProcess, QNetworkReply, 
QSerialPort, plus all the non-Qt code).

> > I think there are easier ways to do this, so I don't think it makes sense
> > to accept this suggestion.
> 
> Yes one can peek() at buffer but as I've tried to state this approach
> has some drawbacks (from performance point of view).  I can also
> understand that maintainers are worried about growing complexity and
> maintenance burden but honestly I don't see any growth in these from
> options B and C above.  These are internal, no testing for them is
> needed, they don't interfere with any other functionality etc.

I think you can help a lot by fixing some of the inefficiencies in peek(). That 
should help you quite a bit already. It won't be ideal, but it will be better. 
Or instead open the device in unbuffered mode and buffer yourself.

I understand the value of countTo(), but it can spiral out of control really 
quickly. After we add the one containing a single byte delimiter, we'll get 
people asking for:
 - string searching (like CRLF)
 - regular 

Re: [Development] Enhancement to QIODevice?

[Andrzej Ostruszka]

On 09/14/2015 12:43 PM, André Somers wrote
> I guess that also means that your countTo() would need to be virtual,
> right? That would not be bc.
No.  This would be just additional functionality of the 
QIODevicePrivateLinearBuffer.
Since this buffer is available from QIODevicePrivate which itself is 
available to all descendants of QIODevice I could implement what I want 
by subclassing QSerialPort.
> Would it not make sense to just add a non-virtual getter and a setter
> for the delimitation marker in the QIODevice API, instead of hard-coding
> '\n'? Obviously you would default that to '\n', but for your usecase you
> would then set it to '\r' and everything else remain as-is...
Yes, that's another option.  This way we still would be doubly searching 
for the delimiter (once in canReadLine and second time in readLine) but 
I can live with that :)

Regards
Andrzej
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Andrzej Ostruszka]

On 09/15/2015 03:53 AM, Thiago Macieira wrote:
> On Monday 14 September 2015 12:25:20 Andrzej Ostruszka wrote:
>> So what I'm aiming to is to make the "line" (or more generally "packet")
>> termination a bit more flexible, that is to allow user to specify the
>> delimiter.  Currently I think the only available solution for my wish is
>> double buffering - that is to introduce my own buffer that will suck
>> everything from QIODevice and allow searching for other delimiter but
>> I'd like to do better than that.
> You could use peek() to search the buffer, then read() exactly as much as you
> really need.
I understand that we are talking about 
QIODevicePrivateLinearBuffer::peek() here.

In your approach I'd be copying data twice, but I can eliminate one with 
peek()ing
directly to the output buffer and later skip()ping in successful case.  
However in case
when delimiter is not yet available I'd have unnecessary copying.
And while peek()ing I'd have to use the min(max size of my output 
buffer, size())
which would mean that I'd be copying "overlapping" parts more than once.

This is suboptimal.

Should I drop this proposal?  From other replies I gather that I'm not 
the only one
that would welcome this or similar enhancement.

Regards
Andrzej
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[André Somers]

Op 14-9-2015 om 12:25 schreef Andrzej Ostruszka:
> Hello all,
>
> I'd like to ask for possibility to enhance a bit QIODevice.
>
> My problem/wish is following.  I'm using QSerialPort and (as all
> QIODevice-s) it is tailored for reading in "lines" ('\n') which is fine
> since most of the time this is exactly what I need.  But I just happen
> to have to deal with an older lock-in device which communicates via
> messages terminated by '\r'.
>
> So what I'm aiming to is to make the "line" (or more generally "packet")
> termination a bit more flexible, that is to allow user to specify the
> delimiter.  Currently I think the only available solution for my wish is
> double buffering - that is to introduce my own buffer that will suck
> everything from QIODevice and allow searching for other delimiter but
> I'd like to do better than that.
>
>   From now on I'll talk about QIODevice only since QSerialPort is
> inheriting from it for the bulk of functionality (and this could also
> benefit QIODevice).
>
> Since '\n' is just hardcoded in QIODevice (actually in buffer used in
> its private part) one option would be to add it as last optional
> parameter to canReadLine() and readLine() with default value '\n'. What
> do you think?
>
> I'm afraid a bit that you might object here:
> - we don't want to change that low level functionality to just fit some
> very specific and rare case (true my example is rare - but I can find
> other aplications like e.g. reading pipe delimited values or other)
> - this would not be backward compatible (although I don't why)
> - being low level would "naturally" push this interface change to all
> classes that inherit from QIODevice and this would be significant change
> - in addition to changing code we would have to also change documentation
>
> So the other option that I'd like for you to consider is to enhance
> buffer used for reading in QIODevicePrivate.
>
> Minimal change that I could come up with is:
> --8<---
> diff --git i/src/corelib/io/qiodevice_p.h w/src/corelib/io/qiodevice_p.h
> index 56a89ab..090e551 100644
> --- i/src/corelib/io/qiodevice_p.h
> +++ w/src/corelib/io/qiodevice_p.h
> @@ -143,6 +143,10 @@ public:
>bool canReadLine() const {
>return memchr(first, '\n', len);
>}
> +qint64 countTo(char c) {
> +char* pos =  static_cast(memchr(first, c, len));
> +return pos ? 1+(pos-first) : 0;
> +}
>void ungetChar(char c) {
>if (first == buf) {
>// underflow, the existing valid data needs to move to the en
> --8<---
> countTo() returns number of bytes one would have to read() in order to
> have first delimiter copied to user data structures (that is it is
> inclusive) and if delimiter is not available than it returns 0.
>
> By using this I can then subclass QSerialPort and implement what I want
> without double buffering.  That way I can also avoid double searching
> for delimiter [1].
I guess that also means that your countTo() would need to be virtual, 
right? That would not be bc.

Would it not make sense to just add a non-virtual getter and a setter 
for the delimitation marker in the QIODevice API, instead of hard-coding 
'\n'? Obviously you would default that to '\n', but for your usecase you 
would then set it to '\r' and everything else remain as-is...

André

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Florian Bruhin]

* Andrzej Ostruszka  [2015-09-14 12:25:20 +0200]:
>  From now on I'll talk about QIODevice only since QSerialPort is 
> inheriting from it for the bulk of functionality (and this could also 
> benefit QIODevice).
> 
> Since '\n' is just hardcoded in QIODevice (actually in buffer used in 
> its private part) one option would be to add it as last optional 
> parameter to canReadLine() and readLine() with default value '\n'. What 
> do you think?

As an electronics engineer using Qt, this would definitely make my
life easier sometimes.

I don't think this is about "protocols using weird line-endings" -
it's about the fact there are many sensible choices for low-level
protocols to signal frames, not only \n. As an example, I was dealing
with a protocol (via QSerialPort) which uses STX (start of text,
ASCII 0x02) as delimiter.

Florian

-- 
http://www.the-compiler.org | m...@the-compiler.org (Mail/XMPP)
   GPG: 916E B0C8 FD55 A072 | http://the-compiler.org/pubkey.asc
 I love long mails! | http://email.is-not-s.ms/


pgpGaGeEpB6Fq.pgp
Description: PGP signature
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Oswald Buddenhagen]

On Mon, Sep 14, 2015 at 12:25:20PM +0200, Andrzej Ostruszka wrote:
> My problem/wish is following.  I'm using QSerialPort and (as all 
> QIODevice-s) it is tailored for reading in "lines" ('\n') which is fine 
> since most of the time this is exactly what I need.  But I just happen 
> to have to deal with an older lock-in device which communicates via 
> messages terminated by '\r'.
> 
just \r, without a subsequent \n? that's old-style mac. i'm not sure we
want to add complexity to support such legacy cases.

> So what I'm aiming to is to make the "line" (or more generally "packet") 
> termination a bit more flexible, that is to allow user to specify the 
> delimiter.
> 
i'd endorse making CRLF conversions available on linux as well, as the
current limitation to windows has bothered me more than once. i wouldn't
go further than that.

i expect opinions from thiago and alex t. :)

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Matthew Woehlke]

On 2015-09-14 06:25, Andrzej Ostruszka wrote:
> I'd like to ask for possibility to enhance a bit QIODevice.
> 
> My problem/wish is following.  I'm using QSerialPort and (as all 
> QIODevice-s) it is tailored for reading in "lines" ('\n') which is fine 
> since most of the time this is exactly what I need.  But I just happen 
> to have to deal with an older lock-in device which communicates via 
> messages terminated by '\r'.
> 
> So what I'm aiming to is to make the "line" (or more generally "packet") 
> termination a bit more flexible [...]

This sounds like an opportunity to discuss with the folks working on CAN
bus how to make QIODevice more generally friendly to protocols that
communicate with some form of "packet", for whatever that definition
happens to be. QUdpSocket could presumably benefit also.

-- 
Matthew

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

Re: [Development] Enhancement to QIODevice?

[Thiago Macieira]

On Monday 14 September 2015 12:25:20 Andrzej Ostruszka wrote:
> So what I'm aiming to is to make the "line" (or more generally "packet") 
> termination a bit more flexible, that is to allow user to specify the 
> delimiter.  Currently I think the only available solution for my wish is 
> double buffering - that is to introduce my own buffer that will suck 
> everything from QIODevice and allow searching for other delimiter but 
> I'd like to do better than that.

You could use peek() to search the buffer, then read() exactly as much as you 
really need.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development

[Development] Enhancement to QIODevice?

[Andrzej Ostruszka]

Hello all,

I'd like to ask for possibility to enhance a bit QIODevice.

My problem/wish is following.  I'm using QSerialPort and (as all 
QIODevice-s) it is tailored for reading in "lines" ('\n') which is fine 
since most of the time this is exactly what I need.  But I just happen 
to have to deal with an older lock-in device which communicates via 
messages terminated by '\r'.

So what I'm aiming to is to make the "line" (or more generally "packet") 
termination a bit more flexible, that is to allow user to specify the 
delimiter.  Currently I think the only available solution for my wish is 
double buffering - that is to introduce my own buffer that will suck 
everything from QIODevice and allow searching for other delimiter but 
I'd like to do better than that.

 From now on I'll talk about QIODevice only since QSerialPort is 
inheriting from it for the bulk of functionality (and this could also 
benefit QIODevice).

Since '\n' is just hardcoded in QIODevice (actually in buffer used in 
its private part) one option would be to add it as last optional 
parameter to canReadLine() and readLine() with default value '\n'. What 
do you think?

I'm afraid a bit that you might object here:
- we don't want to change that low level functionality to just fit some 
very specific and rare case (true my example is rare - but I can find 
other aplications like e.g. reading pipe delimited values or other)
- this would not be backward compatible (although I don't why)
- being low level would "naturally" push this interface change to all 
classes that inherit from QIODevice and this would be significant change
- in addition to changing code we would have to also change documentation

So the other option that I'd like for you to consider is to enhance 
buffer used for reading in QIODevicePrivate.

Minimal change that I could come up with is:
--8<---
diff --git i/src/corelib/io/qiodevice_p.h w/src/corelib/io/qiodevice_p.h
index 56a89ab..090e551 100644
--- i/src/corelib/io/qiodevice_p.h
+++ w/src/corelib/io/qiodevice_p.h
@@ -143,6 +143,10 @@ public:
  bool canReadLine() const {
  return memchr(first, '\n', len);
  }
+qint64 countTo(char c) {
+char* pos =  static_cast(memchr(first, c, len));
+return pos ? 1+(pos-first) : 0;
+}
  void ungetChar(char c) {
  if (first == buf) {
  // underflow, the existing valid data needs to move to the en
--8<---
countTo() returns number of bytes one would have to read() in order to 
have first delimiter copied to user data structures (that is it is 
inclusive) and if delimiter is not available than it returns 0.

By using this I can then subclass QSerialPort and implement what I want 
without double buffering.  That way I can also avoid double searching 
for delimiter [1].

Does this kind of suggestion/change has any chance of going through.  In 
other words: would it be a waste of time for me to setup dev env and 
gerrit to try to push something like this?

Best regards
Andrzej Ostruszka

[1] This is "a problem" with the current interface since usually 
QIODevice::canReadLine() and QIODevice::readLine() are used in tandem 
and in readyRead() callback one checks for "canReadLine" before invoking 
readLine() in order to not block if the whole line is not yet available. 
  This pattern means that we are doubly searching for the delimiter.
___
Development mailing list
Development@qt-project.org
http://lists.qt-project.org/mailman/listinfo/development