[PATCH] libstdc++: istreambuf_iterator proxy (was: keep attached streambuf)

[François Dumont]

On 03/10/2017 22:39, Petr Ovtchenkov wrote:
> On Thu, 28 Sep 2017 13:38:06 +0100
> Jonathan Wakely<jwakely@redhat.com>  wrote:
>
>> On 28/09/17 15:06 +0300, Petr Ovtchenkov wrote:
>>> On Thu, 28 Sep 2017 11:34:25 +0100
>>> Jonathan Wakely<jwakely@redhat.com>  wrote:
>>>>> +  VERIFY(i == std::istreambuf_iterator<char>());
>>>>> +
>>>>> +  VERIFY(memcmp(b, r, 36) == 0);
>>>>> +
>>>>> +  s << q;
>>>>> +  VERIFY(!s.fail());
>>>>> +
>>>>> +  copy_n(i, 36, r);
>>>> This is undefined behaviour. The end-of-stream iterator value cannot
>>>> be dereferenced.
>>> Within this test istreambuf_iterator in eof state never dereferenced.
>> That is quite implementation dependent.
>>
>> The libc++ and VC++ implementations fail this test, because once an
>> istreambuf_iterator has been detected to reach end-of-stream it
>> doesn't get "reset" by changes to the streambuf.
> If we will keep even "unspecified" behaviour same, then bug fix/drawback
> removing become extremely hard: it should be identified as drawback
> in all libs almost simultaneously.
>
>> The libc++ implementation crashes, because operator== on an
>> end-of-stream iterator sets its streambuf* to null, and any further
>> increment or dereference will segfault.
>>
>> So this is testing something that other implementations don't support,
>> and isn't justifiable from the standard.
> I will use N4687 as reference.
>
> 27.2.3 par.2 Table 95:
>
> ++r
>
> Requires: r is dereferenceable. Postconditions: r is dereferenceable or r is
> past-the-end; any copies of the previous value of r are no longer required
> either to be dereferenceable or to be in the domain of ==.
>
> (void)r++ equivalent to (void)++r
>
> *r++
>
> { T tmp = *r;
> ++r;
> return tmp; }
>
> [BTW, you see that r++ without dereference has no sense, and even more,
>
>    copies of the previous
>    value of r are no longer
>    required either to be
>    dereferenceable or to be in
>    the domain of ==.
>
> >From this follow, that postfix increment operator shouldn't return
> istreambuf_iterator.
> ]
>
>>> The test itself simulate "stop and go" istream usage.
>>> stringstream is convenient for behaviuor illustration, but in "real life"
>>> I can assume socket or tty on this place.
>> At the very minimum we should have a comment in the test explaining
>> how it relies on non-standard, non-portable behaviour.
>>
>> But I'd prefer to avoid introducing more divergence from other
>> implementations.
> Standard itself say nothting about "stop and go" scenario.
> At least I don't see any words pro or contra.
> But current implementation block usage of istreambuf_iterator
> with underlying streams like socket or tty, so istreambuf_iterator become
> almost useless structure for practice.
Why not creating a new istreambuf_iterator each time you need to check 
that streambuf is not in eof anymore ?

> We have three issues with istreambuf_iterator:
>    - debug-dependent behaviour
Fixed.
>    - EOL of istreambuf_iterator when it reach EOF (but this not mean
>      EOL of associated streambuf)
Controversial.
>    - postfix increment operator return istreambuf_iterator, but here
>      expected restricted type, that accept only dereference, if it possible.
I agree that we need to fix this last point too.

Consider this code:

 Â  std::istringstream inf("abc");
 Â  std::istreambuf_iterator<char> j(inf), eof;
 Â  std::istreambuf_iterator<char> i = j++;

 Â  assert( *i == 'a' );

At this point it looks like i is pointing to 'a' but then when you do:

std::string str(i, eof);

you have:
assert( str == "ac" );

We jump other the 'b'.

We could improve the situation by adding a debug assertion that _M_c is 
eof when pre-increment is being used or by changing semantic of 
pre-increment to only call sbumpc if _M_c is eof. But then we would need 
to consider _M_c in find overload and in some other places in the lib I 
think.

Rather than going through this complicated path I agree with Petr that 
we need to simply implement the solution advised by the Standard with 
the nested proxy type.

This is what I have done in the attached patch in a naive way. Do we 
need to have abi compatibility here ? If so I'll rework it.

This patch will make libstdc++ pass the llvm test. I even duplicate it 
on our side with a small refinement to check for the return value of the 
proxy::operator*().

François

-------------- next part --------------
A non-text attachment was scrubbed...
Name: istreambuf_iterator_proxy.patch
Type: text/x-patch
Size: 4243 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20171006/ef25c912/attachment.bin>