Re: [PATCH] streambuf_iterator: avoid debug-dependent behaviour

[François Dumont <frs dot dumont at gmail dot com>]

On 30/08/2017 07:05, Petr Ovtchenkov wrote:
> On Tue, 29 Aug 2017 22:02:14 +0200
> François Dumont <frs.dumont@gmail.com> wrote:
>
> > Hi
> >
> >       You should perhaps justify why you want to do those changes.
> Because behaviour under debug should correspond to non-debug variant
> as close as possible,
I agree with you even if you don't need to remove the debug checks to do so.
>   if you mean this part:
>
> @@ -136,9 +136,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         istreambuf_iterator&
>         operator++()
>         {
> -	__glibcxx_requires_cond(!_M_at_eof(),
> -				_M_message(__gnu_debug::__msg_inc_istreambuf)
> -				._M_iterator(*this));
>   	if (_M_sbuf)
>   	  {
>   	    _M_sbuf->sbumpc();
>
> and this:
> @@ -151,14 +148,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         istreambuf_iterator
>         operator++(int)
>         {
> -	__glibcxx_requires_cond(!_M_at_eof(),
> -				_M_message(__gnu_debug::__msg_inc_istreambuf)
> -				._M_iterator(*this));
> +        _M_get();
>   
>   	istreambuf_iterator __old = *this;
>   	if (_M_sbuf)
>   	  {
> -	    __old._M_c = _M_sbuf->sbumpc();
> +	    _M_sbuf->sbumpc();
>   	    _M_c = traits_type::eof();
>   	  }
>   	return __old;
> The second part explicit return __old in "old" state.
> The
> -	    __old._M_c = _M_sbuf->sbumpc();
> work correctly because __old and "current" use same _M_sbuf.
Agree again, this assignment to __old._M_c is useless in your proposal 
(or in mine). Maybe not in current implementation if you use it++ right 
after you generated the iterator. But in this case it means that you 
increment iterator without checking if it eof.
> This part
> @@ -177,18 +172,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         _M_get() const
>         {
>   	const int_type __eof = traits_type::eof();
> -	int_type __ret = __eof;
> -	if (_M_sbuf)
> -	  {
> -	    if (!traits_type::eq_int_type(_M_c, __eof))
> -	      __ret = _M_c;
> -	    else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
> -					       __eof))
> -	      _M_c = __ret;
> -	    else
> -	      _M_sbuf = 0;
> -	  }
> -	return __ret;
> +	if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
> +          _M_c = _M_sbuf->sgetc();
> +	return _M_c;
>         }
> remove drop _M_sbuf if EOF happens.
> Illustration:
> <snip>
>    stringstream ss;
>
>    ss << one;
>
>    isrteambuf_iterator<char>(ss) i;
>    copy_n( i, 10, b );
>    ++i; // EOF reached
>
>    ss << two;
>    copy_n( i, 10, b ); // impossible, if _M_sbuf = 0; when EOF reached before
>    ++i;
> </snip>

Thanks for this illustration, I don't know what to think about it in 
terms of Standard conformity.

> > On 24/08/2017 11:57, Petr Ovtchenkov wrote:
> > > Explicit do sgetc from associated streambuf. Avoid debug-dependent
> > > sgetc (within _M_at_eof()):
> > >
> > >          __glibcxx_requires_cond(!_M_at_eof(),
> > >                                  _M_message(__gnu_debug::__msg_inc_istreambuf)
> > >                                  ._M_iterator(*this));
> > >
> > > Increment operators not require not-eof precoditions.
> > >
> > > Don't unlink associated streambuf if eof detected (in _M_get).
> > Why ? Are you working with a streambuf_type implementation that can
> > return eof at some moment and later return something else ?
> >
> > I don't think istreambuf_iterator has been designed to work with such a
> > streambuf_type implementation. Your streambuf_type should block until it
> > fetches data, just do it in a background thread if you don't want to
> > block all your application while waiting for it.
> >
> > However your request made me consider this implementation, I wonder why
> > it is done this way. Especially why the mutable keywords ?
> >
> > I would have expected an implementation more consistent with the
> > istream_iterator implementation reading current character on
> > construction like in the attached patch.
> >
> > Is it because of the _GLIBCXX_USE_NOEXCEPT constraint on the
> > constructors ? Can sgetc() throw ?
> >
> > François
> What attached file mean? Is it _you_ suggestion/point of view?
> What code we discuss in this case?
My question are for current implementation which is surprisingly 
inconsistent with the istream_iterator one.