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

[Petr Ovtchenkov <ptr at void-ptr dot info>]

On Fri, 6 Oct 2017 18:01:36 +0200
François Dumont <frs.dumont@gmail.com> wrote:

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

It's strange question. Because EOL (end-of-life) for istreambuf_iterator
object after EOF of associated streambuf 

  - not directly follow from standard, just follow from (IMO wrong)
    implementations; and this is a balk for useful usage of istreambuf_iterator;
  - violate expectations from point of view of C++ objects life cycle;
  - require destruction and construction of istreambuf_iterator
    object after check for equality with istreambuf_iterator()
    (strange trick).

> 
> > We have three issues with istreambuf_iterator:
> >    - debug-dependent behaviour
> Fixed.

+	__glibcxx_requires_cond(_M_sbuf,
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
vs

+	__glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));

and

+	__glibcxx_requires_cond(_M_sbuf
+				&& !traits_type::eq_int_type(_M_c,traits_type::eof()),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
vs

+	__glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));

I'm insist on the first variant. It free from code that may lead to different
behaviour between debug/non-debug (like _M_sbuf->sgetc()).



> >    - 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" );

No. I mean that in last (my) suggestion ([PATCH])

   std::istreambuf_iterator<char> i = j++;

is impossible ("impossible" is in aggree with standard).
So test test01() in 2.cc isn't correct.

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

--

  - ptr