[PATCH] streambuf_iterator: avoid debug-dependent behaviour

[PATCH] streambuf_iterator: avoid debug-dependent behaviour

[Petr Ovtchenkov]

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

Clean logic in postfix increment operator.
---
 libstdc++-v3/include/bits/streambuf_iterator.h | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 2230e94..cff3b69 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -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();
@@ -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;
@@ -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;
       }
 
       bool
-- 
2.10.1

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

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

Hi

     You should perhaps justify why you want to do those changes.

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


[-- Attachment #2: istreambuf_iterator.h.patch --]
[-- Type: text/x-patch, Size: 3102 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..d73fedf 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,8 +94,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // the "end of stream" iterator value.
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
-      mutable streambuf_type*	_M_sbuf;
-      mutable int_type		_M_c;
+      streambuf_type*	_M_sbuf;
+      int_type		_M_c;
 
     public:
       ///  Construct end of input stream iterator.
@@ -103,18 +103,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_sbuf(0), _M_c(traits_type::eof()) { }
 
 #if __cplusplus >= 201103L
-      istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+      istreambuf_iterator(const istreambuf_iterator&) = default;
 
       ~istreambuf_iterator() = default;
 #endif
 
       ///  Construct start of input stream iterator.
       istreambuf_iterator(istream_type& __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof())
+      {
+	if (_M_sbuf)
+	  _M_get();
+      }
 
       ///  Construct start of streambuf iterator.
       istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s), _M_c(traits_type::eof())
+      {
+	if (_M_sbuf)
+	  _M_get();
+      }
 
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -129,7 +137,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 				_M_message(__gnu_debug::__msg_deref_istreambuf)
 				._M_iterator(*this));
 #endif
-	return traits_type::to_char_type(_M_get());
+	return traits_type::to_char_type(_M_c);
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
@@ -142,7 +150,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	if (_M_sbuf)
 	  {
 	    _M_sbuf->sbumpc();
-	    _M_c = traits_type::eof();
+	    _M_get();
 	  }
 	return *this;
       }
@@ -159,7 +167,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	if (_M_sbuf)
 	  {
 	    __old._M_c = _M_sbuf->sbumpc();
-	    _M_c = traits_type::eof();
+	    _M_get();
 	  }
 	return __old;
       }
@@ -174,28 +182,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
     private:
       int_type
-      _M_get() const
+      _M_get()
       {
-	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;
+	_M_c = _M_sbuf->sgetc();
+	if (_M_at_eof())
+	  _M_sbuf = 0;
+	return _M_c;
       }
 
       bool
       _M_at_eof() const
       {
 	const int_type __eof = traits_type::eof();
-	return traits_type::eq_int_type(_M_get(), __eof);
+	return traits_type::eq_int_type(_M_c, __eof);
       }
     };
 

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

[Petr Ovtchenkov]

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

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>


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

--

  - ptr

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

[François Dumont]

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.

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

[Jonathan Wakely]

On 24/08/17 12:57 +0300, 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).
>
>Clean logic in postfix increment operator.

I find it very hard to understand the reasons for this patch. What
you've written above is too terse for me.

Are you fixing a bug? If so, do you have a testcase that demonstrates
the problem, and is fixed by these changes?

Is this just refactoring, without changing behaviour?

Finally, and very importantly, do you have a copyright assignment for
changes to GCC? See https://gcc.gnu.org/contribute.html#legal

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

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]

Hi

Following this report I had a closer look to this debug-dependent 
behavior which I agree about.

I have added some checks to 2.cc as shown in the patch. Initially the 
test was failing because:
/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc:45: 
void test02(): Assertion 'old != istrb_eos' failed.

This was coming from the __old._M_c = _M_sbuf->sbumpc() line in the 
post-increment operator. The post-increment is supposed to return a copy 
of the current iterator which this assignment is compromising. So I try 
to remove the assignment and test failed later:

/home/fdt/dev/gcc/git/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc:104: 
void test02(): Assertion 'c == slit01[i]' failed.

However, run in debug mode, it worked fine.

The test is failing in normal mode because the iterator instantiation 
doesn't read the streambuf and without the previous __old._M_c 
assignment _M_c is still pointing to eos. In debug mode, _M_at_eof() 
initialize the iterator and so it work nicely.

So for those reasons I would like to propose attached patch. 
Additionally it gets rid of the mutable usage. And I also remove a 
useless noexcept qualification on the defaulted copy constructor.

François

On 01/09/2017 11:10, Jonathan Wakely wrote:
> On 24/08/17 12:57 +0300, 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).
>>
>> Clean logic in postfix increment operator.
>
> I find it very hard to understand the reasons for this patch. What
> you've written above is too terse for me.
>
> Are you fixing a bug? If so, do you have a testcase that demonstrates
> the problem, and is fixed by these changes?
>
> Is this just refactoring, without changing behaviour?
>
> Finally, and very importantly, do you have a copyright assignment for
> changes to GCC? See https://gcc.gnu.org/contribute.html#legal
>
>
>


[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5166 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..27d0725 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,8 +94,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // the "end of stream" iterator value.
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
-      mutable streambuf_type*	_M_sbuf;
-      mutable int_type		_M_c;
+      streambuf_type*	_M_sbuf;
+      int_type		_M_c;
 
     public:
       ///  Construct end of input stream iterator.
@@ -103,18 +103,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_sbuf(0), _M_c(traits_type::eof()) { }
 
 #if __cplusplus >= 201103L
-      istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+      istreambuf_iterator(const istreambuf_iterator&) = default;
 
       ~istreambuf_iterator() = default;
 #endif
 
       ///  Construct start of input stream iterator.
       istreambuf_iterator(istream_type& __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof())
+      {
+	if (_M_sbuf)
+	  _M_get();
+      }
 
       ///  Construct start of streambuf iterator.
       istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s), _M_c(traits_type::eof())
+      {
+	if (_M_sbuf)
+	  _M_get();
+      }
 
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -122,28 +130,27 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       char_type
       operator*() const
       {
+	int_type __c = _M_get();
+
 #ifdef _GLIBCXX_DEBUG_PEDANTIC
 	// Dereferencing a past-the-end istreambuf_iterator is a
 	// libstdc++ extension
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(!_S_at_eof(__c),
 				_M_message(__gnu_debug::__msg_deref_istreambuf)
 				._M_iterator(*this));
 #endif
-	return traits_type::to_char_type(_M_get());
+	return traits_type::to_char_type(__c);
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf,
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
-	if (_M_sbuf)
-	  {
-	    _M_sbuf->sbumpc();
-	    _M_c = traits_type::eof();
-	  }
+	_M_sbuf->sbumpc();
+	_M_get();
 	return *this;
       }
 
@@ -151,16 +158,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
-				_M_message(__gnu_debug::__msg_inc_istreambuf)
-				._M_iterator(*this));
-
 	istreambuf_iterator __old = *this;
-	if (_M_sbuf)
-	  {
-	    __old._M_c = _M_sbuf->sbumpc();
-	    _M_c = traits_type::eof();
-	  }
+	++(*this);
 	return __old;
       }
 
@@ -176,26 +175,31 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       int_type
       _M_get() const
       {
-	const int_type __eof = traits_type::eof();
-	int_type __ret = __eof;
+	if (!_S_at_eof(_M_c))
+	  return _M_c;
+
 	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;
+	  return _M_sbuf->sgetc();
+	return _M_c;
+      }
+
+      void
+      _M_get()
+      {
+	_M_c = _M_sbuf->sgetc();
+	if (_S_at_eof(_M_c))
+	  _M_sbuf = 0;
       }
 
       bool
       _M_at_eof() const
+      { return _S_at_eof(_M_get()); }
+
+      static bool
+      _S_at_eof(int_type __c)
       {
 	const int_type __eof = traits_type::eof();
-	return traits_type::eq_int_type(_M_get(), __eof);
+	return traits_type::eq_int_type(__c, __eof);
       }
     };
 
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index b81f4d4..498dc1b 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
 
 void test02(void)
 {
-
   typedef std::istreambuf_iterator<char> cistreambuf_iter;
-  typedef cistreambuf_iter::streambuf_type cstreambuf_type;
   const char slit01[] = "playa hermosa, liberia, guanacaste";
   std::string str01(slit01);
   std::istringstream istrs00(str01);
@@ -35,10 +33,18 @@ void test02(void)
 
   // ctor sanity checks
   cistreambuf_iter istrb_it01(istrs00);
-  cistreambuf_iter istrb_it02;
-  std::string tmp(istrb_it01, istrb_it02); 
+  cistreambuf_iter istrb_eos;
+  VERIFY( istrb_it01 != istrb_eos );
+
+  std::string tmp(istrb_it01, istrb_eos);
   VERIFY( tmp == str01 );
 
+  VERIFY( istrb_it01 != istrb_eos );
+  VERIFY( *istrb_it01 == 'p' );
+  cistreambuf_iter old = istrb_it01++;
+  VERIFY( old != istrb_eos );
+  VERIFY( istrb_it01 == istrb_eos );
+
   cistreambuf_iter istrb_it03(0);
   cistreambuf_iter istrb_it04;
   VERIFY( istrb_it03 == istrb_it04 );

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

[Petr Ovtchenkov]

-gcc-patches 

On Thu, 7 Sep 2017 23:02:15 +0200
François Dumont <frs.dumont@gmail.com> wrote:

> +	_M_c = _M_sbuf->sgetc();
> +	if (_S_at_eof(_M_c))
> +	  _M_sbuf = 0;

_M_sbuf = 0; <--- Is not what I axpect here.

Suggestions will be later, after we finish copyright assignment for
changes procedure (in progress).

WBR,

--

   - ptr

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

[François Dumont]

On 08/09/2017 07:47, Petr Ovtchenkov wrote:
> -gcc-patches
>
> On Thu, 7 Sep 2017 23:02:15 +0200
> François Dumont <frs.dumont@gmail.com> wrote:
>
>> +	_M_c = _M_sbuf->sgetc();
>> +	if (_S_at_eof(_M_c))
>> +	  _M_sbuf = 0;
> _M_sbuf = 0; <--- Is not what I expect here.

I know but I didn't say that I agree with all your patch.

I only agree with your remark about current implementation being 
debug-dependant which I have been able to prove with a test case at the 
cost of a small change in the current implementation to fix another problem.

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

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

Hi

     Completing the execution of tests revealed a lot about the current 
implementation.

     The main point of current implementation is to delay as much as 
possible the capture of the current streambuf position. So my original 
proposal capturing state on instantiation was wrong.

     This new proposal concentrate on the debug-dependent code. Debug 
assertions now avoids calling _M_at_eof() which also capture iterator 
state. It also simplifies _M_get() method a little bit like Petr 
proposed but keeping the _M_sbuf reset when reaching eof. Thanks to this 
work I realized that std::find specialization could also be simplified 
by returning a streambuf_iterator which will capture current streambuf 
state on evaluation.

      Note that I haven't been able to create a test case revealing the 
problem. This is more a code quality issue as current code violates the 
principal that debug asserts shouldn't impact object state. AFAIK this 
is noticeable only under gdb.

     Regarding avoiding the reset of _M_sbuf it might be possible,your 
test case could be a good reason to do so. But this is going to be a big 
change for current implementation so don't forget to run all testsuite 
and to consider the std::copy and std::find specializations.

Tested under Linux x86_64, normal and debug modes.

Ok to commit ?

François


On 08/09/2017 07:47, Petr Ovtchenkov wrote:
> -gcc-patches
>
> On Thu, 7 Sep 2017 23:02:15 +0200
> François Dumont <frs.dumont@gmail.com> wrote:
>
>> +	_M_c = _M_sbuf->sgetc();
>> +	if (_S_at_eof(_M_c))
>> +	  _M_sbuf = 0;
> _M_sbuf = 0; <--- Is not what I axpect here.
>
> Suggestions will be later, after we finish copyright assignment for
> changes procedure (in progress).
>
> WBR,
>
> --
>
>     - ptr
>


[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5021 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..ad9c89f 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_sbuf(0), _M_c(traits_type::eof()) { }
 
 #if __cplusplus >= 201103L
-      istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+      istreambuf_iterator(const istreambuf_iterator&) = default;
 
       ~istreambuf_iterator() = default;
 #endif
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       char_type
       operator*() const
       {
+	int_type __c = _M_get();
+
 #ifdef _GLIBCXX_DEBUG_PEDANTIC
 	// Dereferencing a past-the-end istreambuf_iterator is a
 	// libstdc++ extension
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(!_S_at_eof(__c),
 				_M_message(__gnu_debug::__msg_deref_istreambuf)
 				._M_iterator(*this));
 #endif
-	return traits_type::to_char_type(_M_get());
+	return traits_type::to_char_type(__c);
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf
+			&& (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
-	if (_M_sbuf)
-	  {
+
 	_M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
-	  }
 	return *this;
       }
 
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf
+			&& (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	istreambuf_iterator __old = *this;
-	if (_M_sbuf)
-	  {
 	__old._M_c = _M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
-	  }
 	return __old;
       }
 
@@ -176,26 +175,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       int_type
       _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
+	if (_M_sbuf && _S_at_eof(_M_c) && _S_at_eof(_M_c = _M_sbuf->sgetc()))
 	  _M_sbuf = 0;
-	  }
-	return __ret;
+	return _M_c;
       }
 
       bool
       _M_at_eof() const
+      { return _S_at_eof(_M_get()); }
+
+      static bool
+      _S_at_eof(int_type __c)
       {
 	const int_type __eof = traits_type::eof();
-	return traits_type::eq_int_type(_M_get(), __eof);
+	return traits_type::eq_int_type(__c, __eof);
       }
     };
 
@@ -373,13 +366,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::traits_type     traits_type;
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
+      const int_type __eof = traits_type::eof();
 
       if (__first._M_sbuf && !__last._M_sbuf)
 	{
 	  const int_type __ival = traits_type::to_int_type(__val);
 	  streambuf_type* __sb = __first._M_sbuf;
 	  int_type __c = __sb->sgetc();
-	  while (!traits_type::eq_int_type(__c, traits_type::eof())
+	  while (!traits_type::eq_int_type(__c, __eof)
 		 && !traits_type::eq_int_type(__c, __ival))
 	    {
 	      streamsize __n = __sb->egptr() - __sb->gptr();
@@ -396,11 +390,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		__c = __sb->snextc();
 	    }
 
-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
-	    __first._M_c = __c;
-	  else
-	    __first._M_sbuf = 0;
+	  __first._M_c = __eof;
 	}
+
       return __first;
     }
 
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index b81f4d4..e3d99f9 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
 
 void test02(void)
 {
-
   typedef std::istreambuf_iterator<char> cistreambuf_iter;
-  typedef cistreambuf_iter::streambuf_type cstreambuf_type;
   const char slit01[] = "playa hermosa, liberia, guanacaste";
   std::string str01(slit01);
   std::istringstream istrs00(str01);
@@ -35,10 +33,17 @@ void test02(void)
 
   // ctor sanity checks
   cistreambuf_iter istrb_it01(istrs00);
-  cistreambuf_iter istrb_it02;
-  std::string tmp(istrb_it01, istrb_it02); 
+  cistreambuf_iter istrb_eos;
+  VERIFY( istrb_it01 != istrb_eos );
+
+  std::string tmp(istrb_it01, istrb_eos);
   VERIFY( tmp == str01 );
 
+  VERIFY( istrb_it01 != istrb_eos );
+  cistreambuf_iter old = istrb_it01++;
+  VERIFY( old == istrb_eos );
+  VERIFY( istrb_it01 == istrb_eos );
+
   cistreambuf_iter istrb_it03(0);
   cistreambuf_iter istrb_it04;
   VERIFY( istrb_it03 == istrb_it04 );

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

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 2725 bytes --]

Gentle reminder, ok to commit ?

     * include/bits/streambuf_iterator.h
     (istreambuf_iterator<>(const istreambuf_iterator&)): Remove useless
     noexcept qualificatio<n on defaulted implementation.
     (istreambuf_iterator<>::operator*()): Do not capture iterator state
     in Debug assertion.
     (istreambuf_iterator<>::operator++()): Likewise.
     (istreambuf_iterator<>::operator++(int)): Likewise.
     (istreambuf_iterator<>::_M_get()): Simplify implementation.
     (istreambuf_iterator<>::_S_at_eof()): New.
     (istreambuf_iterator<>::_M_at_eof()): Adapt, use latter.
     (find(istreambuf_iterator<>, istreambuf_iterator<>, _CharT)):
     Return an iterator with _M_c set to eof to capture streambuf state
     on evaluation.
     (testsuite/24_iterators/istreambuf_iterator/2.cc): Add checks.

François

On 09/09/2017 22:16, François Dumont wrote:
> Hi
>
>     Completing the execution of tests revealed a lot about the current 
> implementation.
>
>     The main point of current implementation is to delay as much as 
> possible the capture of the current streambuf position. So my original 
> proposal capturing state on instantiation was wrong.
>
>     This new proposal concentrate on the debug-dependent code. Debug 
> assertions now avoids calling _M_at_eof() which also capture iterator 
> state. It also simplifies _M_get() method a little bit like Petr 
> proposed but keeping the _M_sbuf reset when reaching eof. Thanks to 
> this work I realized that std::find specialization could also be 
> simplified by returning a streambuf_iterator which will capture 
> current streambuf state on evaluation.
>
>      Note that I haven't been able to create a test case revealing the 
> problem. This is more a code quality issue as current code violates 
> the principal that debug asserts shouldn't impact object state. AFAIK 
> this is noticeable only under gdb.
>
>     Regarding avoiding the reset of _M_sbuf it might be possible,your 
> test case could be a good reason to do so. But this is going to be a 
> big change for current implementation so don't forget to run all 
> testsuite and to consider the std::copy and std::find specializations.
>
> Tested under Linux x86_64, normal and debug modes.
>
> Ok to commit ?
>
> François
>
>
> On 08/09/2017 07:47, Petr Ovtchenkov wrote:
>> -gcc-patches
>>
>> On Thu, 7 Sep 2017 23:02:15 +0200
>> François Dumont <frs.dumont@gmail.com> wrote:
>>
>>> +    _M_c = _M_sbuf->sgetc();
>>> +    if (_S_at_eof(_M_c))
>>> +      _M_sbuf = 0;
>> _M_sbuf = 0; <--- Is not what I axpect here.
>>
>> Suggestions will be later, after we finish copyright assignment for
>> changes procedure (in progress).
>>
>> WBR,
>>
>> -- 
>>
>>     - ptr
>>
>


[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5021 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..ad9c89f 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_sbuf(0), _M_c(traits_type::eof()) { }
 
 #if __cplusplus >= 201103L
-      istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+      istreambuf_iterator(const istreambuf_iterator&) = default;
 
       ~istreambuf_iterator() = default;
 #endif
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       char_type
       operator*() const
       {
+	int_type __c = _M_get();
+
 #ifdef _GLIBCXX_DEBUG_PEDANTIC
 	// Dereferencing a past-the-end istreambuf_iterator is a
 	// libstdc++ extension
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(!_S_at_eof(__c),
 				_M_message(__gnu_debug::__msg_deref_istreambuf)
 				._M_iterator(*this));
 #endif
-	return traits_type::to_char_type(_M_get());
+	return traits_type::to_char_type(__c);
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf
+			&& (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
-	if (_M_sbuf)
-	  {
+
 	_M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
-	  }
 	return *this;
       }
 
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf
+			&& (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	istreambuf_iterator __old = *this;
-	if (_M_sbuf)
-	  {
 	__old._M_c = _M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
-	  }
 	return __old;
       }
 
@@ -176,26 +175,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       int_type
       _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
+	if (_M_sbuf && _S_at_eof(_M_c) && _S_at_eof(_M_c = _M_sbuf->sgetc()))
 	  _M_sbuf = 0;
-	  }
-	return __ret;
+	return _M_c;
       }
 
       bool
       _M_at_eof() const
+      { return _S_at_eof(_M_get()); }
+
+      static bool
+      _S_at_eof(int_type __c)
       {
 	const int_type __eof = traits_type::eof();
-	return traits_type::eq_int_type(_M_get(), __eof);
+	return traits_type::eq_int_type(__c, __eof);
       }
     };
 
@@ -373,13 +366,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::traits_type     traits_type;
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
+      const int_type __eof = traits_type::eof();
 
       if (__first._M_sbuf && !__last._M_sbuf)
 	{
 	  const int_type __ival = traits_type::to_int_type(__val);
 	  streambuf_type* __sb = __first._M_sbuf;
 	  int_type __c = __sb->sgetc();
-	  while (!traits_type::eq_int_type(__c, traits_type::eof())
+	  while (!traits_type::eq_int_type(__c, __eof)
 		 && !traits_type::eq_int_type(__c, __ival))
 	    {
 	      streamsize __n = __sb->egptr() - __sb->gptr();
@@ -396,11 +390,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		__c = __sb->snextc();
 	    }
 
-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
-	    __first._M_c = __c;
-	  else
-	    __first._M_sbuf = 0;
+	  __first._M_c = __eof;
 	}
+
       return __first;
     }
 
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index b81f4d4..e3d99f9 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
 
 void test02(void)
 {
-
   typedef std::istreambuf_iterator<char> cistreambuf_iter;
-  typedef cistreambuf_iter::streambuf_type cstreambuf_type;
   const char slit01[] = "playa hermosa, liberia, guanacaste";
   std::string str01(slit01);
   std::istringstream istrs00(str01);
@@ -35,10 +33,17 @@ void test02(void)
 
   // ctor sanity checks
   cistreambuf_iter istrb_it01(istrs00);
-  cistreambuf_iter istrb_it02;
-  std::string tmp(istrb_it01, istrb_it02); 
+  cistreambuf_iter istrb_eos;
+  VERIFY( istrb_it01 != istrb_eos );
+
+  std::string tmp(istrb_it01, istrb_eos);
   VERIFY( tmp == str01 );
 
+  VERIFY( istrb_it01 != istrb_eos );
+  cistreambuf_iter old = istrb_it01++;
+  VERIFY( old == istrb_eos );
+  VERIFY( istrb_it01 == istrb_eos );
+
   cistreambuf_iter istrb_it03(0);
   cistreambuf_iter istrb_it04;
   VERIFY( istrb_it03 == istrb_it04 );

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

[Jonathan Wakely]

On 21/09/17 07:46 +0200, François Dumont wrote:
>Gentle reminder, ok to commit ?

No. Could you and Petr please come to an agreement about what is
actually wrong with the current implementation, and agree on a
solution?

Currently you're both just proposing patches that do different things,
without indicating why one patch is better than the other.

I understand that we want to remove the debugmode-dependent behaviour,
but I'd like to see any other changes justified by references to the
standard.
 
>diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
>index b81f4d4..e3d99f9 100644
>--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
>+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
>@@ -25,9 +25,7 @@
> 
> void test02(void)
> {
>-
>   typedef std::istreambuf_iterator<char> cistreambuf_iter;
>-  typedef cistreambuf_iter::streambuf_type cstreambuf_type;
>   const char slit01[] = "playa hermosa, liberia, guanacaste";
>   std::string str01(slit01);
>   std::istringstream istrs00(str01);
>@@ -35,10 +33,17 @@ void test02(void)
> 
>   // ctor sanity checks
>   cistreambuf_iter istrb_it01(istrs00);
>-  cistreambuf_iter istrb_it02;
>-  std::string tmp(istrb_it01, istrb_it02); 
>+  cistreambuf_iter istrb_eos;
>+  VERIFY( istrb_it01 != istrb_eos );
>+
>+  std::string tmp(istrb_it01, istrb_eos);
>   VERIFY( tmp == str01 );
> 
>+  VERIFY( istrb_it01 != istrb_eos );

Why should this condition be true? The std::string constructor
increments the iterator until it reaches the end-of-stream value.

This is true with our current implementation, but that seems like a
bug, not something we want to verify in the testsuite.

>+  cistreambuf_iter old = istrb_it01++;
>+  VERIFY( old == istrb_eos );

This behaviour makes no sense.

>+  VERIFY( istrb_it01 == istrb_eos );
>+
>   cistreambuf_iter istrb_it03(0);
>   cistreambuf_iter istrb_it04;
>   VERIFY( istrb_it03 == istrb_it04 );

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

[Jonathan Wakely]

On 28/09/17 11:50 +0100, Jonathan Wakely wrote:
>On 21/09/17 07:46 +0200, François Dumont wrote:
>>Gentle reminder, ok to commit ?
>
>No. Could you and Petr please come to an agreement about what is
>actually wrong with the current implementation, and agree on a
>solution?
>
>Currently you're both just proposing patches that do different things,
>without indicating why one patch is better than the other.
>
>I understand that we want to remove the debugmode-dependent behaviour,
>but I'd like to see any other changes justified by references to the
>standard.

Here's a test we currently fail, but it looks like we should pass it:
http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp

Do the changes either of you is proposing change this result?

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

[Petr Ovtchenkov]

On Fri, 1 Sep 2017 10:10:37 +0100
Jonathan Wakely <jwakely@redhat.com> wrote:

> On 24/08/17 12:57 +0300, 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).
> >
> >Clean logic in postfix increment operator.
> 
> I find it very hard to understand the reasons for this patch. What
> you've written above is too terse for me.
> 
> Are you fixing a bug? If so, do you have a testcase that demonstrates
> the problem, and is fixed by these changes?

Yes, I think I see a bug and I have testcase (at least I think
that I have it). Looks, that François see this issue too
and suggest patch. But I'm not satisfied with it.

My previous suggestion has side effect, that is not acceptable.
I'm trying to find better solution.

> 
> Is this just refactoring, without changing behaviour?
> 
> Finally, and very importantly, do you have a copyright assignment for
> changes to GCC? See https://gcc.gnu.org/contribute.html#legal

The formal procedure is completed. Thanks.

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

[Petr Ovtchenkov]

On Thu, 21 Sep 2017 21:23:13 +0300
Petr Ovtchenkov <ptr@void-ptr.info> wrote:

> On Fri, 1 Sep 2017 10:10:37 +0100
> Jonathan Wakely <jwakely@redhat.com> wrote:
> 
> > On 24/08/17 12:57 +0300, 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).
> > >
> > >Clean logic in postfix increment operator.
> > 
> > I find it very hard to understand the reasons for this patch. What
> > you've written above is too terse for me.
> > 
> > Are you fixing a bug? If so, do you have a testcase that demonstrates
> > the problem, and is fixed by these changes?
> 
> Yes, I think I see a bug and I have testcase (at least I think
> that I have it). Looks, that François see this issue too
> and suggest patch. But I'm not satisfied with it.
> 
> My previous suggestion has side effect, that is not acceptable.
> I'm trying to find better solution.

https://gcc.gnu.org/ml/libstdc++/2017-09/msg00088.html

[PATCH] libstdc++: istreambuf_iterator keep attached streambuf

[Petr Ovtchenkov]

istreambuf_iterator should not forget about attached
streambuf when it reach EOF.

Checks in debug mode has no infuence more on character
extraction in istreambuf_iterator increment operators.
In this aspect behaviour in debug and non-debug mode
is similar now.

Test for detached srteambuf in istreambuf_iterator:
When istreambuf_iterator reach EOF of istream, it should not
forget about attached streambuf.
From fact "EOF in stream reached" not follow that
stream reach end of life and input operation impossible
more.
---
 libstdc++-v3/include/bits/streambuf_iterator.h     | 41 +++++++--------
 .../24_iterators/istreambuf_iterator/3.cc          | 61 ++++++++++++++++++++++
 2 files changed, 80 insertions(+), 22 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..45c3d89 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf,
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 	if (_M_sbuf)
 	  {
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+	    int_type _tmp =
+#endif
 	    _M_sbuf->sbumpc();
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+	    __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
+				    _M_message(__gnu_debug::__msg_inc_istreambuf)
+				    ._M_iterator(*this));
+#endif
 	    _M_c = traits_type::eof();
 	  }
 	return *this;
@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+        _M_get();
+	__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));
 
 	istreambuf_iterator __old = *this;
 	if (_M_sbuf)
 	  {
-	    __old._M_c = _M_sbuf->sbumpc();
+	    _M_sbuf->sbumpc();
 	    _M_c = traits_type::eof();
 	  }
 	return __old;
@@ -177,18 +187,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;
       }
 
       bool
@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
 
-      if (__first._M_sbuf && !__last._M_sbuf)
+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
 	{
 	  streambuf_type* __sb = __first._M_sbuf;
 	  int_type __c = __sb->sgetc();
@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
 
-      if (__first._M_sbuf && !__last._M_sbuf)
+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
 	{
 	  const int_type __ival = traits_type::to_int_type(__val);
 	  streambuf_type* __sb = __first._M_sbuf;
@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	      else
 		__c = __sb->snextc();
 	    }
-
-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
-	    __first._M_c = __c;
-	  else
-	    __first._M_sbuf = 0;
+	  __first._M_c = __c;
 	}
       return __first;
     }
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
new file mode 100644
index 0000000..803ede4
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
@@ -0,0 +1,61 @@
+// { dg-options "-std=gnu++17" }
+
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+#include <cstring>
+#include <testsuite_hooks.h>
+
+void test03()
+{
+  using namespace std;
+  bool test __attribute__((unused)) = true;
+
+  std::stringstream s;
+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
+  char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
+  char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
+  //          012345678901234567890123456789012345
+  //          0         1         2         3
+  s << b;
+  VERIFY( !s.fail() );
+
+  istreambuf_iterator<char> i(s);
+  copy_n(i, 36, r);
+  ++i; // EOF reached
+  VERIFY(i == std::istreambuf_iterator<char>());
+
+  VERIFY(memcmp(b, r, 36) == 0);
+
+  s << q;
+  VERIFY(!s.fail());
+
+  copy_n(i, 36, r);
+  ++i; // EOF reached
+  VERIFY(i == std::istreambuf_iterator<char>());
+
+  VERIFY(memcmp(q, r, 36) == 0);
+}
+
+int main()
+{
+  test03();
+  return 0;
+}
-- 
2.10.1

Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf

[Jonathan Wakely]

On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
>istreambuf_iterator should not forget about attached
>streambuf when it reach EOF.
>
>Checks in debug mode has no infuence more on character
>extraction in istreambuf_iterator increment operators.
>In this aspect behaviour in debug and non-debug mode
>is similar now.
>
>Test for detached srteambuf in istreambuf_iterator:
>When istreambuf_iterator reach EOF of istream, it should not
>forget about attached streambuf.
From fact "EOF in stream reached" not follow that
>stream reach end of life and input operation impossible
>more.
>---
> libstdc++-v3/include/bits/streambuf_iterator.h     | 41 +++++++--------
> .../24_iterators/istreambuf_iterator/3.cc          | 61 ++++++++++++++++++++++
> 2 files changed, 80 insertions(+), 22 deletions(-)
> create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>
>diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
>index f0451b1..45c3d89 100644
>--- a/libstdc++-v3/include/bits/streambuf_iterator.h
>+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
>@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       istreambuf_iterator&
>       operator++()
>       {
>-	__glibcxx_requires_cond(!_M_at_eof(),
>+	__glibcxx_requires_cond(_M_sbuf,
> 				_M_message(__gnu_debug::__msg_inc_istreambuf)
> 				._M_iterator(*this));
> 	if (_M_sbuf)
> 	  {
>+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>+	    int_type _tmp =

_tmp is not a reserved name, this needs to be __tmp.

I'm still reviewing the rest, to understand what observable behaviour
this changes, and how it differs from the patch François sent.

Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf

[Jonathan Wakely]

On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
>istreambuf_iterator should not forget about attached
>streambuf when it reach EOF.
>
>Checks in debug mode has no infuence more on character
>extraction in istreambuf_iterator increment operators.
>In this aspect behaviour in debug and non-debug mode
>is similar now.
>
>Test for detached srteambuf in istreambuf_iterator:
>When istreambuf_iterator reach EOF of istream, it should not
>forget about attached streambuf.
From fact "EOF in stream reached" not follow that
>stream reach end of life and input operation impossible
>more.
>---
> libstdc++-v3/include/bits/streambuf_iterator.h     | 41 +++++++--------
> .../24_iterators/istreambuf_iterator/3.cc          | 61 ++++++++++++++++++++++
> 2 files changed, 80 insertions(+), 22 deletions(-)
> create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>
>diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
>index f0451b1..45c3d89 100644
>--- a/libstdc++-v3/include/bits/streambuf_iterator.h
>+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
>@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       istreambuf_iterator&
>       operator++()
>       {
>-	__glibcxx_requires_cond(!_M_at_eof(),
>+	__glibcxx_requires_cond(_M_sbuf,
> 				_M_message(__gnu_debug::__msg_inc_istreambuf)
> 				._M_iterator(*this));
> 	if (_M_sbuf)
> 	  {
>+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>+	    int_type _tmp =
>+#endif
> 	    _M_sbuf->sbumpc();
>+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>+	    __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
>+				    _M_message(__gnu_debug::__msg_inc_istreambuf)
>+				    ._M_iterator(*this));
>+#endif
> 	    _M_c = traits_type::eof();
> 	  }
> 	return *this;
>@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       istreambuf_iterator
>       operator++(int)
>       {
>-	__glibcxx_requires_cond(!_M_at_eof(),
>+        _M_get();
>+	__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));
>
> 	istreambuf_iterator __old = *this;
> 	if (_M_sbuf)
> 	  {
>-	    __old._M_c = _M_sbuf->sbumpc();
>+	    _M_sbuf->sbumpc();
> 	    _M_c = traits_type::eof();
> 	  }
> 	return __old;
>@@ -177,18 +187,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;
>       }
>
>       bool
>@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
>       typedef typename traits_type::int_type               int_type;
>
>-      if (__first._M_sbuf && !__last._M_sbuf)
>+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> 	{
> 	  streambuf_type* __sb = __first._M_sbuf;
> 	  int_type __c = __sb->sgetc();
>@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
>       typedef typename traits_type::int_type               int_type;
>
>-      if (__first._M_sbuf && !__last._M_sbuf)
>+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> 	{
> 	  const int_type __ival = traits_type::to_int_type(__val);
> 	  streambuf_type* __sb = __first._M_sbuf;
>@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	      else
> 		__c = __sb->snextc();
> 	    }
>-
>-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
>-	    __first._M_c = __c;
>-	  else
>-	    __first._M_sbuf = 0;
>+	  __first._M_c = __c;
> 	}
>       return __first;
>     }
>diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>new file mode 100644
>index 0000000..803ede4
>--- /dev/null
>+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>@@ -0,0 +1,61 @@
>+// { dg-options "-std=gnu++17" }
>+
>+// Copyright (C) 2017 Free Software Foundation, Inc.
>+//
>+// This file is part of the GNU ISO C++ Library.  This library is free
>+// software; you can redistribute it and/or modify it under the
>+// terms of the GNU General Public License as published by the
>+// Free Software Foundation; either version 3, or (at your option)
>+// any later version.
>+
>+// This library is distributed in the hope that it will be useful,
>+// but WITHOUT ANY WARRANTY; without even the implied warranty of
>+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+// GNU General Public License for more details.
>+
>+// You should have received a copy of the GNU General Public License along
>+// with this library; see the file COPYING3.  If not see
>+// <http://www.gnu.org/licenses/>.
>+
>+#include <algorithm>
>+#include <sstream>
>+#include <iterator>
>+#include <cstring>
>+#include <testsuite_hooks.h>
>+
>+void test03()
>+{
>+  using namespace std;
>+  bool test __attribute__((unused)) = true;
>+
>+  std::stringstream s;
>+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
>+  char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
>+  char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
>+  //          012345678901234567890123456789012345
>+  //          0         1         2         3
>+  s << b;
>+  VERIFY( !s.fail() );
>+
>+  istreambuf_iterator<char> i(s);
>+  copy_n(i, 36, r);
>+  ++i; // EOF reached
>+  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.

Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf

[Petr Ovtchenkov]

On Thu, 28 Sep 2017 11:34:25 +0100
Jonathan Wakely <jwakely@redhat.com> wrote:

> On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
> >istreambuf_iterator should not forget about attached
> >streambuf when it reach EOF.
> >
> >Checks in debug mode has no infuence more on character
> >extraction in istreambuf_iterator increment operators.
> >In this aspect behaviour in debug and non-debug mode
> >is similar now.
> >
> >Test for detached srteambuf in istreambuf_iterator:
> >When istreambuf_iterator reach EOF of istream, it should not
> >forget about attached streambuf.
> From fact "EOF in stream reached" not follow that
> >stream reach end of life and input operation impossible
> >more.
> >---
> > libstdc++-v3/include/bits/streambuf_iterator.h     | 41 +++++++--------
> > .../24_iterators/istreambuf_iterator/3.cc          | 61 ++++++++++++++++++++++
> > 2 files changed, 80 insertions(+), 22 deletions(-)
> > create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >
> >diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
> >b/libstdc++-v3/include/bits/streambuf_iterator.h index f0451b1..45c3d89 100644
> >--- a/libstdc++-v3/include/bits/streambuf_iterator.h
> >+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> >@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >       istreambuf_iterator&
> >       operator++()
> >       {
> >-	__glibcxx_requires_cond(!_M_at_eof(),
> >+	__glibcxx_requires_cond(_M_sbuf,
> > 				_M_message(__gnu_debug::__msg_inc_istreambuf)
> > 				._M_iterator(*this));
> > 	if (_M_sbuf)
> > 	  {
> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
> >+	    int_type _tmp =
> >+#endif
> > 	    _M_sbuf->sbumpc();
> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
> >+	    __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
> >+				    _M_message(__gnu_debug::__msg_inc_istreambuf)
> >+				    ._M_iterator(*this));
> >+#endif
> > 	    _M_c = traits_type::eof();
> > 	  }
> > 	return *this;
> >@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >       istreambuf_iterator
> >       operator++(int)
> >       {
> >-	__glibcxx_requires_cond(!_M_at_eof(),
> >+        _M_get();
> >+	__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));
> >
> > 	istreambuf_iterator __old = *this;
> > 	if (_M_sbuf)
> > 	  {
> >-	    __old._M_c = _M_sbuf->sbumpc();
> >+	    _M_sbuf->sbumpc();
> > 	    _M_c = traits_type::eof();
> > 	  }
> > 	return __old;
> >@@ -177,18 +187,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;
> >       }
> >
> >       bool
> >@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
> >       typedef typename traits_type::int_type               int_type;
> >
> >-      if (__first._M_sbuf && !__last._M_sbuf)
> >+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> > 	{
> > 	  streambuf_type* __sb = __first._M_sbuf;
> > 	  int_type __c = __sb->sgetc();
> >@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
> >       typedef typename traits_type::int_type               int_type;
> >
> >-      if (__first._M_sbuf && !__last._M_sbuf)
> >+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> > 	{
> > 	  const int_type __ival = traits_type::to_int_type(__val);
> > 	  streambuf_type* __sb = __first._M_sbuf;
> >@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > 	      else
> > 		__c = __sb->snextc();
> > 	    }
> >-
> >-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
> >-	    __first._M_c = __c;
> >-	  else
> >-	    __first._M_sbuf = 0;
> >+	  __first._M_c = __c;
> > 	}
> >       return __first;
> >     }
> >diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc new file mode 100644
> >index 0000000..803ede4
> >--- /dev/null
> >+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >@@ -0,0 +1,61 @@
> >+// { dg-options "-std=gnu++17" }
> >+
> >+// Copyright (C) 2017 Free Software Foundation, Inc.
> >+//
> >+// This file is part of the GNU ISO C++ Library.  This library is free
> >+// software; you can redistribute it and/or modify it under the
> >+// terms of the GNU General Public License as published by the
> >+// Free Software Foundation; either version 3, or (at your option)
> >+// any later version.
> >+
> >+// This library is distributed in the hope that it will be useful,
> >+// but WITHOUT ANY WARRANTY; without even the implied warranty of
> >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >+// GNU General Public License for more details.
> >+
> >+// You should have received a copy of the GNU General Public License along
> >+// with this library; see the file COPYING3.  If not see
> >+// <http://www.gnu.org/licenses/>.
> >+
> >+#include <algorithm>
> >+#include <sstream>
> >+#include <iterator>
> >+#include <cstring>
> >+#include <testsuite_hooks.h>
> >+
> >+void test03()
> >+{
> >+  using namespace std;
> >+  bool test __attribute__((unused)) = true;
> >+
> >+  std::stringstream s;
> >+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
> >+  char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
> >+  char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
> >+  //          012345678901234567890123456789012345
> >+  //          0         1         2         3
> >+  s << b;
> >+  VERIFY( !s.fail() );
> >+
> >+  istreambuf_iterator<char> i(s);
> >+  copy_n(i, 36, r);
> >+  ++i; // EOF reached
> >+  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.
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.

debugmode-dependent behaviour is also issue in this patch;
I don't suggest it as a separate patch because solutions are intersect.

WBR,

--

   - ptr

Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf

[Jonathan Wakely]

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:
>
>> On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
>> >istreambuf_iterator should not forget about attached
>> >streambuf when it reach EOF.
>> >
>> >Checks in debug mode has no infuence more on character
>> >extraction in istreambuf_iterator increment operators.
>> >In this aspect behaviour in debug and non-debug mode
>> >is similar now.
>> >
>> >Test for detached srteambuf in istreambuf_iterator:
>> >When istreambuf_iterator reach EOF of istream, it should not
>> >forget about attached streambuf.
>> From fact "EOF in stream reached" not follow that
>> >stream reach end of life and input operation impossible
>> >more.
>> >---
>> > libstdc++-v3/include/bits/streambuf_iterator.h     | 41 +++++++--------
>> > .../24_iterators/istreambuf_iterator/3.cc          | 61 ++++++++++++++++++++++
>> > 2 files changed, 80 insertions(+), 22 deletions(-)
>> > create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>> >
>> >diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
>> >b/libstdc++-v3/include/bits/streambuf_iterator.h index f0451b1..45c3d89 100644
>> >--- a/libstdc++-v3/include/bits/streambuf_iterator.h
>> >+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
>> >@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >       istreambuf_iterator&
>> >       operator++()
>> >       {
>> >-	__glibcxx_requires_cond(!_M_at_eof(),
>> >+	__glibcxx_requires_cond(_M_sbuf,
>> > 				_M_message(__gnu_debug::__msg_inc_istreambuf)
>> > 				._M_iterator(*this));
>> > 	if (_M_sbuf)
>> > 	  {
>> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>> >+	    int_type _tmp =
>> >+#endif
>> > 	    _M_sbuf->sbumpc();
>> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
>> >+	    __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
>> >+				    _M_message(__gnu_debug::__msg_inc_istreambuf)
>> >+				    ._M_iterator(*this));
>> >+#endif
>> > 	    _M_c = traits_type::eof();
>> > 	  }
>> > 	return *this;
>> >@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >       istreambuf_iterator
>> >       operator++(int)
>> >       {
>> >-	__glibcxx_requires_cond(!_M_at_eof(),
>> >+        _M_get();
>> >+	__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));
>> >
>> > 	istreambuf_iterator __old = *this;
>> > 	if (_M_sbuf)
>> > 	  {
>> >-	    __old._M_c = _M_sbuf->sbumpc();
>> >+	    _M_sbuf->sbumpc();
>> > 	    _M_c = traits_type::eof();
>> > 	  }
>> > 	return __old;
>> >@@ -177,18 +187,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;
>> >       }
>> >
>> >       bool
>> >@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
>> >       typedef typename traits_type::int_type               int_type;
>> >
>> >-      if (__first._M_sbuf && !__last._M_sbuf)
>> >+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
>> > 	{
>> > 	  streambuf_type* __sb = __first._M_sbuf;
>> > 	  int_type __c = __sb->sgetc();
>> >@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
>> >       typedef typename traits_type::int_type               int_type;
>> >
>> >-      if (__first._M_sbuf && !__last._M_sbuf)
>> >+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
>> > 	{
>> > 	  const int_type __ival = traits_type::to_int_type(__val);
>> > 	  streambuf_type* __sb = __first._M_sbuf;
>> >@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> > 	      else
>> > 		__c = __sb->snextc();
>> > 	    }
>> >-
>> >-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
>> >-	    __first._M_c = __c;
>> >-	  else
>> >-	    __first._M_sbuf = 0;
>> >+	  __first._M_c = __c;
>> > 	}
>> >       return __first;
>> >     }
>> >diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>> >b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc new file mode 100644
>> >index 0000000..803ede4
>> >--- /dev/null
>> >+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
>> >@@ -0,0 +1,61 @@
>> >+// { dg-options "-std=gnu++17" }
>> >+
>> >+// Copyright (C) 2017 Free Software Foundation, Inc.
>> >+//
>> >+// This file is part of the GNU ISO C++ Library.  This library is free
>> >+// software; you can redistribute it and/or modify it under the
>> >+// terms of the GNU General Public License as published by the
>> >+// Free Software Foundation; either version 3, or (at your option)
>> >+// any later version.
>> >+
>> >+// This library is distributed in the hope that it will be useful,
>> >+// but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> >+// GNU General Public License for more details.
>> >+
>> >+// You should have received a copy of the GNU General Public License along
>> >+// with this library; see the file COPYING3.  If not see
>> >+// <http://www.gnu.org/licenses/>.
>> >+
>> >+#include <algorithm>
>> >+#include <sstream>
>> >+#include <iterator>
>> >+#include <cstring>
>> >+#include <testsuite_hooks.h>
>> >+
>> >+void test03()
>> >+{
>> >+  using namespace std;
>> >+  bool test __attribute__((unused)) = true;

This variable should be removed, we don't need these variables any
more.

>> >+  std::stringstream s;
>> >+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
>> >+  char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
>> >+  char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
>> >+  //          012345678901234567890123456789012345
>> >+  //          0         1         2         3
>> >+  s << b;
>> >+  VERIFY( !s.fail() );
>> >+
>> >+  istreambuf_iterator<char> i(s);
>> >+  copy_n(i, 36, r);
>> >+  ++i; // EOF reached
>> >+  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.

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.

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

>debugmode-dependent behaviour is also issue in this patch;
>I don't suggest it as a separate patch because solutions are intersect.

Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf

[Petr Ovtchenkov]

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:
> >
> >> On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote:
> >> >istreambuf_iterator should not forget about attached
> >> >streambuf when it reach EOF.
> >> >
> >> >Checks in debug mode has no infuence more on character
> >> >extraction in istreambuf_iterator increment operators.
> >> >In this aspect behaviour in debug and non-debug mode
> >> >is similar now.
> >> >
> >> >Test for detached srteambuf in istreambuf_iterator:
> >> >When istreambuf_iterator reach EOF of istream, it should not
> >> >forget about attached streambuf.
> >> From fact "EOF in stream reached" not follow that
> >> >stream reach end of life and input operation impossible
> >> >more.
> >> >---
> >> > libstdc++-v3/include/bits/streambuf_iterator.h     | 41 +++++++--------
> >> > .../24_iterators/istreambuf_iterator/3.cc          | 61 ++++++++++++++++++++++
> >> > 2 files changed, 80 insertions(+), 22 deletions(-)
> >> > create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >> >
> >> >diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
> >> >b/libstdc++-v3/include/bits/streambuf_iterator.h index f0451b1..45c3d89 100644
> >> >--- a/libstdc++-v3/include/bits/streambuf_iterator.h
> >> >+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> >> >@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> >       istreambuf_iterator&
> >> >       operator++()
> >> >       {
> >> >-	__glibcxx_requires_cond(!_M_at_eof(),
> >> >+	__glibcxx_requires_cond(_M_sbuf,
> >> > 				_M_message(__gnu_debug::__msg_inc_istreambuf)
> >> > 				._M_iterator(*this));
> >> > 	if (_M_sbuf)
> >> > 	  {
> >> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
> >> >+	    int_type _tmp =
> >> >+#endif
> >> > 	    _M_sbuf->sbumpc();
> >> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC
> >> >+	    __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_type::eof()),
> >> >+				    _M_message(__gnu_debug::__msg_inc_istreambuf)
> >> >+				    ._M_iterator(*this));
> >> >+#endif
> >> > 	    _M_c = traits_type::eof();
> >> > 	  }
> >> > 	return *this;
> >> >@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> >       istreambuf_iterator
> >> >       operator++(int)
> >> >       {
> >> >-	__glibcxx_requires_cond(!_M_at_eof(),
> >> >+        _M_get();
> >> >+	__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));
> >> >
> >> > 	istreambuf_iterator __old = *this;
> >> > 	if (_M_sbuf)
> >> > 	  {
> >> >-	    __old._M_c = _M_sbuf->sbumpc();
> >> >+	    _M_sbuf->sbumpc();
> >> > 	    _M_c = traits_type::eof();
> >> > 	  }
> >> > 	return __old;
> >> >@@ -177,18 +187,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;
> >> >       }
> >> >
> >> >       bool
> >> >@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> >       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
> >> >       typedef typename traits_type::int_type               int_type;
> >> >
> >> >-      if (__first._M_sbuf && !__last._M_sbuf)
> >> >+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> >> > 	{
> >> > 	  streambuf_type* __sb = __first._M_sbuf;
> >> > 	  int_type __c = __sb->sgetc();
> >> >@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> >       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
> >> >       typedef typename traits_type::int_type               int_type;
> >> >
> >> >-      if (__first._M_sbuf && !__last._M_sbuf)
> >> >+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
> >> > 	{
> >> > 	  const int_type __ival = traits_type::to_int_type(__val);
> >> > 	  streambuf_type* __sb = __first._M_sbuf;
> >> >@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >> > 	      else
> >> > 		__c = __sb->snextc();
> >> > 	    }
> >> >-
> >> >-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
> >> >-	    __first._M_c = __c;
> >> >-	  else
> >> >-	    __first._M_sbuf = 0;
> >> >+	  __first._M_c = __c;
> >> > 	}
> >> >       return __first;
> >> >     }
> >> >diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >> >b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc new file mode 100644
> >> >index 0000000..803ede4
> >> >--- /dev/null
> >> >+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
> >> >@@ -0,0 +1,61 @@
> >> >+// { dg-options "-std=gnu++17" }
> >> >+
> >> >+// Copyright (C) 2017 Free Software Foundation, Inc.
> >> >+//
> >> >+// This file is part of the GNU ISO C++ Library.  This library is free
> >> >+// software; you can redistribute it and/or modify it under the
> >> >+// terms of the GNU General Public License as published by the
> >> >+// Free Software Foundation; either version 3, or (at your option)
> >> >+// any later version.
> >> >+
> >> >+// This library is distributed in the hope that it will be useful,
> >> >+// but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> >+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> >+// GNU General Public License for more details.
> >> >+
> >> >+// You should have received a copy of the GNU General Public License along
> >> >+// with this library; see the file COPYING3.  If not see
> >> >+// <http://www.gnu.org/licenses/>.
> >> >+
> >> >+#include <algorithm>
> >> >+#include <sstream>
> >> >+#include <iterator>
> >> >+#include <cstring>
> >> >+#include <testsuite_hooks.h>
> >> >+
> >> >+void test03()
> >> >+{
> >> >+  using namespace std;
> >> >+  bool test __attribute__((unused)) = true;
> 
> This variable should be removed, we don't need these variables any
> more.

Not a problem, if we will have consensus in principle.

> 
> >> >+  std::stringstream s;
> >> >+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
> >> >+  char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
> >> >+  char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
> >> >+  //          012345678901234567890123456789012345
> >> >+  //          0         1         2         3
> >> >+  s << b;
> >> >+  VERIFY( !s.fail() );
> >> >+
> >> >+  istreambuf_iterator<char> i(s);
> >> >+  copy_n(i, 36, r);
> >> >+  ++i; // EOF reached
> >> >+  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.

> 
> >debugmode-dependent behaviour is also issue in this patch;
> >I don't suggest it as a separate patch because solutions are intersect.

We have three issues with istreambuf_iterator:

  - debug-dependent behaviour
  - EOL of istreambuf_iterator when it reach EOF (but this not mean
    EOL of associated streambuf)
  - postfix increment operator return istreambuf_iterator, but here
    expected restricted type, that accept only dereference, if it possible.

[PATCH v2] libstdc++: istreambuf_iterator keep attached streambuf

[Petr Ovtchenkov]

istreambuf_iterator should not forget about attached
streambuf when it reach EOF.

Checks in debug mode has no infuence more on character
extraction in istreambuf_iterator increment operators.
In this aspect behaviour in debug and non-debug mode
is similar now.

Test for detached srteambuf in istreambuf_iterator:
When istreambuf_iterator reach EOF of istream, it should not
forget about attached streambuf.
From fact "EOF in stream reached" not follow that
stream reach end of life and input operation impossible
more.

postfix increment (r++) return isb_iterator_proxy, due to

  copies of the previous value of r are no longer
  required either to be dereferenceable or to be in
  the domain of ==.

i.e. type that usable only for dereference and extraction
"previous" character.
---
 libstdc++-v3/include/bits/streambuf_iterator.h     | 60 ++++++++++++--------
 .../24_iterators/istreambuf_iterator/3.cc          | 64 ++++++++++++++++++++++
 2 files changed, 100 insertions(+), 24 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..b71bdd2 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -97,6 +97,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       mutable streambuf_type*	_M_sbuf;
       mutable int_type		_M_c;
 
+      class isb_iterator_proxy
+      {
+          friend class istreambuf_iterator;
+      private:
+          isb_iterator_proxy(int_type c) :
+              _M_c(c)
+              { }
+          int_type _M_c;
+
+      public:
+          char_type
+          operator*() const
+              { return traits_type::to_char_type(_M_c); }
+      };
+
     public:
       ///  Construct end of input stream iterator.
       _GLIBCXX_CONSTEXPR istreambuf_iterator() _GLIBCXX_USE_NOEXCEPT
@@ -136,29 +151,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf,
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 	if (_M_sbuf)
 	  {
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+	    int_type __tmp =
+#endif
 	    _M_sbuf->sbumpc();
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+	    __glibcxx_requires_cond(!traits_type::eq_int_type(__tmp,traits_type::eof()),
+				    _M_message(__gnu_debug::__msg_inc_istreambuf)
+				    ._M_iterator(*this));
+#endif
 	    _M_c = traits_type::eof();
 	  }
 	return *this;
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
-      istreambuf_iterator
+      isb_iterator_proxy
       operator++(int)
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+        _M_get();
+	__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));
 
-	istreambuf_iterator __old = *this;
+	isb_iterator_proxy __old(_M_c);
 	if (_M_sbuf)
 	  {
-	    __old._M_c = _M_sbuf->sbumpc();
+	    _M_sbuf->sbumpc();
 	    _M_c = traits_type::eof();
 	  }
 	return __old;
@@ -177,18 +202,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;
       }
 
       bool
@@ -339,7 +355,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
 
-      if (__first._M_sbuf && !__last._M_sbuf)
+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
 	{
 	  streambuf_type* __sb = __first._M_sbuf;
 	  int_type __c = __sb->sgetc();
@@ -374,7 +390,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
 
-      if (__first._M_sbuf && !__last._M_sbuf)
+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
 	{
 	  const int_type __ival = traits_type::to_int_type(__val);
 	  streambuf_type* __sb = __first._M_sbuf;
@@ -395,11 +411,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	      else
 		__c = __sb->snextc();
 	    }
-
-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
-	    __first._M_c = __c;
-	  else
-	    __first._M_sbuf = 0;
+	  __first._M_c = __c;
 	}
       return __first;
     }
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
new file mode 100644
index 0000000..5792e0d
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
@@ -0,0 +1,64 @@
+// { dg-options "-std=gnu++17" }
+
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+#include <cstring>
+#include <testsuite_hooks.h>
+
+void test03()
+{
+  using namespace std;
+
+  std::stringstream s;
+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
+  char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
+  char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
+  //          012345678901234567890123456789012345
+  //          0         1         2         3
+  s << b;
+  VERIFY( !s.fail() );
+
+  istreambuf_iterator<char> i(s);
+  copy_n(i, 36, r);
+  ++i; // EOF reached
+  VERIFY(i == std::istreambuf_iterator<char>());
+
+  VERIFY(memcmp(b, r, 36) == 0);
+
+  s << q;
+  VERIFY(!s.fail());
+
+  copy_n(i, 36, r);
+  ++i; // EOF reached
+  VERIFY(i == std::istreambuf_iterator<char>());
+
+  VERIFY(memcmp(q, r, 36) == 0);
+
+  s << 'Q';
+
+  VERIFY(*i++ == 'Q');
+}
+
+int main()
+{
+  test03();
+  return 0;
+}
-- 
2.10.1

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

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 4365 bytes --]

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


[-- Attachment #2: istreambuf_iterator_proxy.patch --]
[-- Type: text/x-patch, Size: 4243 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 64b8cfd..a556fce 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,12 +95,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
       mutable streambuf_type*	_M_sbuf;
-      int_type			_M_c;
 
     public:
       ///  Construct end of input stream iterator.
       _GLIBCXX_CONSTEXPR istreambuf_iterator() _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(0), _M_c(traits_type::eof()) { }
+      : _M_sbuf() { }
 
 #if __cplusplus >= 201103L
       istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
@@ -110,11 +109,33 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       ///  Construct start of input stream iterator.
       istreambuf_iterator(istream_type& __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s.rdbuf()) { }
 
       ///  Construct start of streambuf iterator.
       istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s) { }
+
+      class proxy
+      {
+	friend class istreambuf_iterator;
+
+	proxy(streambuf_type* __buf, int_type __c)
+	: _M_sbuf(__buf), _M_c(__c)
+	{ }
+
+	streambuf_type*	_M_sbuf;
+	int_type	_M_c;
+
+      public:
+	char_type
+	operator*() const
+	{ return traits_type::to_char_type(_M_c); }
+      };
+
+      /// Construct start of streambuf iterator.
+      istreambuf_iterator(const proxy& __prx) _GLIBCXX_USE_NOEXCEPT
+      : _M_sbuf(__prx._M_sbuf)
+      { }
 
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -138,29 +159,23 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	_M_sbuf->sbumpc();
-	_M_c = traits_type::eof();
 	return *this;
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
-      istreambuf_iterator
+      proxy
       operator++(int)
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(_M_sbuf && !_S_is_eof(_M_sbuf->sgetc()),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
-	istreambuf_iterator __old = *this;
-	__old._M_c = _M_sbuf->sbumpc();
-	_M_c = traits_type::eof();
-	return __old;
+	return proxy(_M_sbuf, _M_sbuf->sbumpc());
       }
 
       // _GLIBCXX_RESOLVE_LIB_DEFECTS
@@ -175,8 +190,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       int_type
       _M_get() const
       {
-	int_type __ret = _M_c;
-	if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
+	int_type __ret = traits_type::eof();
+	if (_M_sbuf && _S_is_eof(__ret = _M_sbuf->sgetc()))
 	  _M_sbuf = 0;
 	return __ret;
       }
@@ -390,8 +405,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	      else
 		__c = __sb->snextc();
 	    }
-
-	  __first._M_c = __eof;
 	}
 
       return __first;
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index 572ea9f..de4b8e4 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -23,7 +23,18 @@
 #include <iterator>
 #include <testsuite_hooks.h>
 
-void test02(void)
+void test01()
+{
+  std::istringstream inf("abc");
+  std::istreambuf_iterator<char> j(inf);
+  std::istreambuf_iterator<char> i = j++;
+
+  VERIFY( i != std::istreambuf_iterator<char>() );
+  VERIFY( *i == 'b' );
+  VERIFY( *j++ == 'b' );
+}
+
+void test02()
 {
   typedef std::istreambuf_iterator<char> cistreambuf_iter;
   const char slit01[] = "playa hermosa, liberia, guanacaste";
@@ -111,6 +122,7 @@ void test02(void)
 
 int main()
 {
+  test01();
   test02();
   return 0;
 }

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

[Petr Ovtchenkov]

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

Re: [PATCH] libstdc++: istreambuf_iterator proxy

[François Dumont]

On 06/10/2017 20:00, Petr Ovtchenkov wrote:
> 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()).

The first patch fixed the impact of the Debug mode on the 
istreambuf_iterator state. This kind of difference is classical with 
Debug mode, this mode usually introduces additional calls to some 
operators or in this case to sgetc.

We have no choice here as otherwise we wouldn't detect that the iterator 
is an eof one. I might propose another patch after this one that could 
allow to limit the check to _M_sbuf not being null.

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

It is correct as this constructor:
+      /// Construct start of streambuf iterator.
+      istreambuf_iterator(const proxy& __prx) _GLIBCXX_USE_NOEXCEPT
+      : _M_sbuf(__prx._M_sbuf)
+      { }

is defined by the Standard. This is why the llvm test is fine too.

What is illegal is rather something like:
++(j++);

Re: [PATCH] libstdc++: istreambuf_iterator proxy

[Petr Ovtchenkov]

On Sun, 8 Oct 2017 16:59:39 +0200
François Dumont <frs.dumont@gmail.com> wrote:

> ...
> >>
> >> 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.
> 
> It is correct as this constructor:
> +      /// Construct start of streambuf iterator.
> +      istreambuf_iterator(const proxy& __prx) _GLIBCXX_USE_NOEXCEPT
> +      : _M_sbuf(__prx._M_sbuf)
> +      { }
> 
> is defined by the Standard. This is why the llvm test is fine too.

Yes, you right here.

But in

+  std::istringstream inf("abc");
+  std::istreambuf_iterator<char> j(inf);
+  std::istreambuf_iterator<char> i = j++;
+
+  VERIFY( i != std::istreambuf_iterator<char>() );
+  VERIFY( *i == 'b' );
+  VERIFY( *j++ == 'b' );

the last two lines

+  VERIFY( *i == 'b' );
+  VERIFY( *j++ == 'b' );

is a check of the implementation-specific behaviour, because of

  ++r     ... 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

(i is a copy of "previous" value of j)


From other side, for ctor from proxy

   istreambuf_iterator(const proxy& p) noexcept;
5  Effects: Initializes sbuf_ with p.sbuf_.

that mean that reference of istreambuf_iterator to basic_streambuf
(sbuf_) has some meaning. proxy may be dereferenced or used
to produce istreambuf_iterator that may be used for == (but not
for dereference itself ;)).

Re: [PATCH] libstdc++: istreambuf_iterator proxy

[Petr Ovtchenkov]

On Sun, 8 Oct 2017 16:59:39 +0200
François Dumont <frs.dumont@gmail.com> wrote:

> ...
> >>> 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()).
> 
> The first patch fixed the impact of the Debug mode on the 
> istreambuf_iterator state. This kind of difference is classical with 
> Debug mode, this mode usually introduces additional calls to some 
> operators or in this case to sgetc.
> 

The key is "_M_sbuf->sgetc()" and "_S_is_eof" that may call ->sgetc() too.
This may lead to difference in debug/non-debug behaviour.
Solution without such difference exist and was presented here.

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

[Jonathan Wakely]

On 06/10/17 18:01 +0200, François Dumont wrote:
>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'.

Right, but this code isn't required to work. These are Input
Iterators. The fact that incrementing j affects other copies of the
iterator is expected.


>We could improve the situation by adding a debug assertion that _M_c 
>is eof when pre-increment is being used

Hmm, interesting idea.

>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*().

I agree with the earlier analysis that the libc++ test is not required
to work (i've reported this to the libc++ maintainers and expect them
to move it to their nonportable test directory).  So we don't need to
change our implementation to make it pass.

Make tests less istreambuf_iterator implementation dependent

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

Hi

     I just committed attached patch as trivial.

     Those tests were highly istreambuf_iterator implementation, it is 
the result of the call to money_get<>::get which is pointing immediately 
beyond the last character recognized to quote Standard words.

2017-09-27  François Dumont  <fdumont@gcc.gnu.org>

     * testsuite/22_locale/money_get/get/char/22131.cc: Make test less
     istreambuf_iterator implementation dependent.
     * testsuite/22_locale/money_get/get/wchar_t/22131.cc: Likewise.

François


[-- Attachment #2: tests.patch --]
[-- Type: text/x-patch, Size: 1218 bytes --]

diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/char/22131.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/char/22131.cc
index 6f363ef..5a05817 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/char/22131.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/char/22131.cc
@@ -67,7 +67,7 @@ void test01()
   fmt2.imbue(loc);
   InIt ibeg2(fmt2);
   err2 = ios_base::goodbit;
-  mg.get(ibeg2, iend2, intl, fmt2, err2, val2);
+  ibeg2 = mg.get(ibeg2, iend2, intl, fmt2, err2, val2);
   VERIFY( err2 == ios_base::failbit );
   VERIFY( *ibeg2 == '#' );
   VERIFY( val2 == "" );
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/22131.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/22131.cc
index 3830cc3..0626e3c 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/22131.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/22131.cc
@@ -67,7 +67,7 @@ void test01()
   fmt2.imbue(loc);
   InIt ibeg2(fmt2);
   err2 = ios_base::goodbit;
-  mg.get(ibeg2, iend2, intl, fmt2, err2, val2);
+  ibeg2 = mg.get(ibeg2, iend2, intl, fmt2, err2, val2);
   VERIFY( err2 == ios_base::failbit );
   VERIFY( *ibeg2 == L'#' );
   VERIFY( val2 == L"" );

Re: Make tests less istreambuf_iterator implementation dependent

[Jonathan Wakely]

On 27/09/17 22:16 +0200, François Dumont wrote:
>Hi
>
>    I just committed attached patch as trivial.
>
>    Those tests were highly istreambuf_iterator implementation, it is 
>the result of the call to money_get<>::get which is pointing 
>immediately beyond the last character recognized to quote Standard 
>words.

But according to the standard's specification for istreambuf_iterator
it makes no difference, because both iterators point to the same
streambuf and share the state.

Re: Make tests less istreambuf_iterator implementation dependent

[François Dumont]

On 28/09/2017 14:12, Jonathan Wakely wrote:
> On 27/09/17 22:16 +0200, François Dumont wrote:
>> Hi
>>
>>     I just committed attached patch as trivial.
>>
>>     Those tests were highly istreambuf_iterator implementation, it is 
>> the result of the call to money_get<>::get which is pointing 
>> immediately beyond the last character recognized to quote Standard 
>> words.
>
> But according to the standard's specification for istreambuf_iterator
> it makes no difference, because both iterators point to the same
> streambuf and share the state.
>
>
Simply adding a:

VERIFY( *ibeg2 == '0' );

before calling money_get<>::get method would have broken the test.

The current istreambuf_iterator implementation capture the current 
streambuf state each time it is tested for eof or evaluated. This is why 
I considered those tests as fragile.

François

Re: Make tests less istreambuf_iterator implementation dependent

[Jonathan Wakely]

On 28/09/17 21:59 +0200, François Dumont wrote:
>On 28/09/2017 14:12, Jonathan Wakely wrote:
>>On 27/09/17 22:16 +0200, François Dumont wrote:
>>>Hi
>>>
>>>    I just committed attached patch as trivial.
>>>
>>>    Those tests were highly istreambuf_iterator implementation, it 
>>>is the result of the call to money_get<>::get which is pointing 
>>>immediately beyond the last character recognized to quote Standard 
>>>words.
>>
>>But according to the standard's specification for istreambuf_iterator
>>it makes no difference, because both iterators point to the same
>>streambuf and share the state.
>>
>>
>Simply adding a:
>
>VERIFY( *ibeg2 == '0' );
>
>before calling money_get<>::get method would have broken the test.
>
>The current istreambuf_iterator implementation capture the current 
>streambuf state each time it is tested for eof or evaluated. This is 
>why I considered those tests as fragile.

Yes, and I think that's non-conforming.

As I said in the other thread, I'd really like to see references to
the standard used to justify any changes to our istreambuf_iterator.

Re: Make tests less istreambuf_iterator implementation dependent

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On 28/09/2017 23:56, Jonathan Wakely wrote:
> On 28/09/17 21:59 +0200, François Dumont wrote:
>>
>> The current istreambuf_iterator implementation capture the current 
>> streambuf state each time it is tested for eof or evaluated. This is 
>> why I considered those tests as fragile.
>
> Yes, and I think that's non-conforming.
>

Good, then we have something to fix.

> As I said in the other thread, I'd really like to see references to
> the standard used to justify any changes to our istreambuf_iterator 

I think _M_c has been introduced to cover this paragraph of the Standard:

24.6.3.1

"1. Class istreambuf_iterator<charT,traits>::proxy is for exposition 
only. An implementation is permit-
ted to provide equivalent functionality without providing a class with 
this name. Class istreambuf_-
iterator<charT, traits>::proxy provides a temporary placeholder as the 
return value of the post-
increment operator (operator++). It keeps the character pointed to by 
the previous value of the iterator for
some possible future access to get the character."

This is why it is being set in operator++(int):

     istreambuf_iterator __old = *this;
     __old._M_c = _M_sbuf->sbumpc();

It is also the reason why libstdc++ fails:
http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp 


It looks like this test is simply not Standard conformant.

I guess at some point _M_c started to be used also to cache the 
streambuf::sgetc resulting in current situation.

In attached patch I limit usage of _M_c to cover 24.6.3.1.1 point and so 
made additional changes to 2.cc test case to demonstrate it.

François


[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5232 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..94d8c7f 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,7 +95,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
       mutable streambuf_type*	_M_sbuf;
-      mutable int_type		_M_c;
+      int_type			_M_c;
 
     public:
       ///  Construct end of input stream iterator.
@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       : _M_sbuf(0), _M_c(traits_type::eof()) { }
 
 #if __cplusplus >= 201103L
-      istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+      istreambuf_iterator(const istreambuf_iterator&) = default;
 
       ~istreambuf_iterator() = default;
 #endif
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       char_type
       operator*() const
       {
+	int_type __c = _M_get();
+
 #ifdef _GLIBCXX_DEBUG_PEDANTIC
 	// Dereferencing a past-the-end istreambuf_iterator is a
 	// libstdc++ extension
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(!_S_at_eof(__c),
 				_M_message(__gnu_debug::__msg_deref_istreambuf)
 				._M_iterator(*this));
 #endif
-	return traits_type::to_char_type(_M_get());
+	return traits_type::to_char_type(__c);
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf &&
+				(!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
-	if (_M_sbuf)
-	  {
+
 	_M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
-	  }
 	return *this;
       }
 
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf &&
+				(!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	istreambuf_iterator __old = *this;
-	if (_M_sbuf)
-	  {
 	__old._M_c = _M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
-	  }
 	return __old;
       }
 
@@ -176,26 +175,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       int_type
       _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
+	int_type __ret = _M_c;
+	if (_M_sbuf && _S_at_eof(__ret) && _S_at_eof(__ret = _M_sbuf->sgetc()))
 	  _M_sbuf = 0;
-	  }
 	return __ret;
       }
 
       bool
       _M_at_eof() const
+      { return _S_at_eof(_M_get()); }
+
+      static bool
+      _S_at_eof(int_type __c)
       {
 	const int_type __eof = traits_type::eof();
-	return traits_type::eq_int_type(_M_get(), __eof);
+	return traits_type::eq_int_type(__c, __eof);
       }
     };
 
@@ -373,13 +367,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::traits_type     traits_type;
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
+      const int_type __eof = traits_type::eof();
 
       if (__first._M_sbuf && !__last._M_sbuf)
 	{
 	  const int_type __ival = traits_type::to_int_type(__val);
 	  streambuf_type* __sb = __first._M_sbuf;
 	  int_type __c = __sb->sgetc();
-	  while (!traits_type::eq_int_type(__c, traits_type::eof())
+	  while (!traits_type::eq_int_type(__c, __eof)
 		 && !traits_type::eq_int_type(__c, __ival))
 	    {
 	      streamsize __n = __sb->egptr() - __sb->gptr();
@@ -396,11 +391,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		__c = __sb->snextc();
 	    }
 
-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
-	    __first._M_c = __c;
-	  else
-	    __first._M_sbuf = 0;
+	  __first._M_c = __eof;
 	}
+
       return __first;
     }
 
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index b81f4d4..572ea9f 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
 
 void test02(void)
 {
-
   typedef std::istreambuf_iterator<char> cistreambuf_iter;
-  typedef cistreambuf_iter::streambuf_type cstreambuf_type;
   const char slit01[] = "playa hermosa, liberia, guanacaste";
   std::string str01(slit01);
   std::istringstream istrs00(str01);
@@ -35,10 +33,14 @@ void test02(void)
 
   // ctor sanity checks
   cistreambuf_iter istrb_it01(istrs00);
-  cistreambuf_iter istrb_it02;
-  std::string tmp(istrb_it01, istrb_it02); 
+  cistreambuf_iter istrb_eos;
+  VERIFY( istrb_it01 != istrb_eos );
+
+  std::string tmp(istrb_it01, istrb_eos);
   VERIFY( tmp == str01 );
 
+  VERIFY( istrb_it01 == istrb_eos );
+
   cistreambuf_iter istrb_it03(0);
   cistreambuf_iter istrb_it04;
   VERIFY( istrb_it03 == istrb_it04 );

Re: Make tests less istreambuf_iterator implementation dependent

[Jonathan Wakely]

On 02/10/17 07:43 +0200, François Dumont wrote:
>On 28/09/2017 23:56, Jonathan Wakely wrote:
>>On 28/09/17 21:59 +0200, François Dumont wrote:
>>>
>>>The current istreambuf_iterator implementation capture the current 
>>>streambuf state each time it is tested for eof or evaluated. This 
>>>is why I considered those tests as fragile.
>>
>>Yes, and I think that's non-conforming.
>>
>
>Good, then we have something to fix.
>
>>As I said in the other thread, I'd really like to see references to
>>the standard used to justify any changes to our istreambuf_iterator
>
>I think _M_c has been introduced to cover this paragraph of the Standard:
>
>24.6.3.1
>
>"1. Class istreambuf_iterator<charT,traits>::proxy is for exposition 
>only. An implementation is permit-
>ted to provide equivalent functionality without providing a class with 
>this name. Class istreambuf_-
>iterator<charT, traits>::proxy provides a temporary placeholder as the 
>return value of the post-
>increment operator (operator++). It keeps the character pointed to by 
>the previous value of the iterator for
>some possible future access to get the character."
>
>This is why it is being set in operator++(int):
>
>    istreambuf_iterator __old = *this;
>    __old._M_c = _M_sbuf->sbumpc();
>
>It is also the reason why libstdc++ fails:
>http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp
>
>
>It looks like this test is simply not Standard conformant.

I think you're right, because it relies on a property that may not be
true:

  any copies of the previous
  value of r are no longer
  required either to be
  dereferenceable or to be in
  the domain of ==.

>I guess at some point _M_c started to be used also to cache the 
>streambuf::sgetc resulting in current situation.

And arguably that is not "equivalent functionality" to returning a
proxy object. Although the standard seems a bit underspecified.

>In attached patch I limit usage of _M_c to cover 24.6.3.1.1 point and 
>so made additional changes to 2.cc test case to demonstrate it.

Doesn't the modified test PASS anyway, even without changing how _M_c
is used?

Generally this looks good (I'm not sure if it addresses Petr's
concerns, but I don't think his "stop and go" usage is valid anyway -
it's certainly not portable or guaranteed by the standard).

>@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       : _M_sbuf(0), _M_c(traits_type::eof()) { }
> 
> #if __cplusplus >= 201103L
>-      istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
>+      istreambuf_iterator(const istreambuf_iterator&) = default;

Please don't remove this.

It's present in the standard, and it ensures we don't accidentally add
a member that makes the default noexcept(false).

> 
>       ~istreambuf_iterator() = default;
> #endif
>@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       char_type
>       operator*() const
>       {
>+	int_type __c = _M_get();
>+
> #ifdef _GLIBCXX_DEBUG_PEDANTIC
> 	// Dereferencing a past-the-end istreambuf_iterator is a
> 	// libstdc++ extension
>-	__glibcxx_requires_cond(!_M_at_eof(),
>+	__glibcxx_requires_cond(!_S_at_eof(__c),
> 				_M_message(__gnu_debug::__msg_deref_istreambuf)
> 				._M_iterator(*this));
> #endif
>-	return traits_type::to_char_type(_M_get());
>+	return traits_type::to_char_type(__c);
>       }
> 
>       /// Advance the iterator.  Calls streambuf.sbumpc().
>       istreambuf_iterator&
>       operator++()
>       {
>-	__glibcxx_requires_cond(!_M_at_eof(),
>+	__glibcxx_requires_cond(_M_sbuf &&
>+				(!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
> 				_M_message(__gnu_debug::__msg_inc_istreambuf)
> 				._M_iterator(*this));
>-	if (_M_sbuf)
>-	  {

This check should be unnecessary, because it's undefined to increment
and end-of-stream iterator, but I wonder if anyone is relying on it
being currently a no-op, rather than crashing.

Let's remove the check, and if it causes too many problems we can
restore it as:

  if (__builtin_expect(_M_sbuf != 0, 1))

>+
> 	_M_sbuf->sbumpc();
> 	_M_c = traits_type::eof();
>-	  }
> 	return *this;
>       }
> 
>@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       istreambuf_iterator
>       operator++(int)
>       {
>-	__glibcxx_requires_cond(!_M_at_eof(),
>+	__glibcxx_requires_cond(_M_sbuf &&
>+				(!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
> 				_M_message(__gnu_debug::__msg_inc_istreambuf)
> 				._M_iterator(*this));
> 
> 	istreambuf_iterator __old = *this;
>-	if (_M_sbuf)
>-	  {
> 	__old._M_c = _M_sbuf->sbumpc();
> 	_M_c = traits_type::eof();
>-	  }
> 	return __old;
>       }
> 
>@@ -176,26 +175,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       int_type
>       _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;

Removing this assignment means that we will call _M_sbuf->sgetc() for
every dereference operation, instead of caching the current value in
_M_c. I think that's probably OK, because it's unlikely that
performance critical code is dereferencing the same iterator
repeatedly without incrementing it.

And getting rid of the mutable member is a Good Thing.

An alternative would be to cache it in operator*() instead of in
_M_get(), which would still give the desired change of not updating it
when _M_get() is used elsewhere. But would still involve setting a
mutable member in a const function.

>-	    else
>+	int_type __ret = _M_c;
>+	if (_M_sbuf && _S_at_eof(__ret) && _S_at_eof(__ret = _M_sbuf->sgetc()))
> 	  _M_sbuf = 0;

It's unfortunate that we still have this assignment to a mutable
member in a const function, but I think it's necessary to avoid an
end-of-stream iterator becoming valid again.

>-	  }
> 	return __ret;
>       }
> 
>       bool
>       _M_at_eof() const
>+      { return _S_at_eof(_M_get()); }
>+
>+      static bool
>+      _S_at_eof(int_type __c)

This should be called _S_is_eof, since it tests if the argument **is**
the EOF character. It doesn't test if the stream is **at** EOF, like
_M_at_eof does.

So with a suitable ChangeLog, the "noexcept" kept on the constructor,
and renaming _S_at_eof to _S_is_eof, this is OK for trunk.

Re: Make tests less istreambuf_iterator implementation dependent

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 7101 bytes --]

On 03/10/2017 16:20, Jonathan Wakely wrote:
> On 02/10/17 07:43 +0200, François Dumont wrote:
>> On 28/09/2017 23:56, Jonathan Wakely wrote:
>>> On 28/09/17 21:59 +0200, François Dumont wrote:
>>>>
>>>> The current istreambuf_iterator implementation capture the current 
>>>> streambuf state each time it is tested for eof or evaluated. This 
>>>> is why I considered those tests as fragile.
>>>
>>> Yes, and I think that's non-conforming.
>>>
>>
>> Good, then we have something to fix.
>>
>>> As I said in the other thread, I'd really like to see references to
>>> the standard used to justify any changes to our istreambuf_iterator
>>
>> I think _M_c has been introduced to cover this paragraph of the 
>> Standard:
>>
>> 24.6.3.1
>>
>> "1. Class istreambuf_iterator<charT,traits>::proxy is for exposition 
>> only. An implementation is permit-
>> ted to provide equivalent functionality without providing a class 
>> with this name. Class istreambuf_-
>> iterator<charT, traits>::proxy provides a temporary placeholder as 
>> the return value of the post-
>> increment operator (operator++). It keeps the character pointed to by 
>> the previous value of the iterator for
>> some possible future access to get the character."
>>
>> This is why it is being set in operator++(int):
>>
>>     istreambuf_iterator __old = *this;
>>     __old._M_c = _M_sbuf->sbumpc();
>>
>> It is also the reason why libstdc++ fails:
>> http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp 
>>
>>
>>
>> It looks like this test is simply not Standard conformant.
>
> I think you're right, because it relies on a property that may not be
> true:
>
>  any copies of the previous
>  value of r are no longer
>  required either to be
>  dereferenceable or to be in
>  the domain of ==.
>
>> I guess at some point _M_c started to be used also to cache the 
>> streambuf::sgetc resulting in current situation.
>
> And arguably that is not "equivalent functionality" to returning a
> proxy object. Although the standard seems a bit underspecified.
>
>> In attached patch I limit usage of _M_c to cover 24.6.3.1.1 point and 
>> so made additional changes to 2.cc test case to demonstrate it.
>
> Doesn't the modified test PASS anyway, even without changing how _M_c
> is used?

No it doesn't because the first check for eof capture the 'a' char which 
is never reseted so after string construction it is still pointing to 
this char and is not an eof iterator. Without the _M_c assignment the 
iterator is still "live" and so keeps on reflecting the streambuf state.

I'll show you some other side effects of this _M_c later.

>
> Generally this looks good (I'm not sure if it addresses Petr's
> concerns, but I don't think his "stop and go" usage is valid anyway -
> it's certainly not portable or guaranteed by the standard).
No it doesn't address Petr's concern who is more focus on the _M_sbuf 
nullification part.
>
>> @@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>       : _M_sbuf(0), _M_c(traits_type::eof()) { }
>>
>> #if __cplusplus >= 201103L
>> -      istreambuf_iterator(const istreambuf_iterator&) noexcept = 
>> default;
>> +      istreambuf_iterator(const istreambuf_iterator&) = default;
>
> Please don't remove this.
>
> It's present in the standard, and it ensures we don't accidentally add
> a member that makes the default noexcept(false).
Ok
>
>> )),
>>                 _M_message(__gnu_debug::__msg_inc_istreambuf)
>>                 ._M_iterator(*this));
>> -    if (_M_sbuf)
>> -      {
>
> This check should be unnecessary, because it's undefined to increment
> and end-of-stream iterator, but I wonder if anyone is relying on it
> being currently a no-op, rather than crashing.
>
> Let's remove the check, and if it causes too many problems we can
> restore it as:
>
>  if (__builtin_expect(_M_sbuf != 0, 1))
>
>> -        else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
>> -                           __eof))
>> -          _M_c = __ret;
>
> Removing this assignment means that we will call _M_sbuf->sgetc() for
> every dereference operation, instead of caching the current value in
> _M_c. I think that's probably OK, because it's unlikely that
> performance critical code is dereferencing the same iterator
> repeatedly without incrementing it.
>
> And getting rid of the mutable member is a Good Thing.
>
> An alternative would be to cache it in operator*() instead of in
> _M_get(), which would still give the desired change of not updating it
> when _M_get() is used elsewhere. But would still involve setting a
> mutable member in a const function.
>
>> -        else
>> +    int_type __ret = _M_c;
>> +    if (_M_sbuf && _S_at_eof(__ret) && _S_at_eof(__ret = 
>> _M_sbuf->sgetc()))
>>       _M_sbuf = 0;
>
> It's unfortunate that we still have this assignment to a mutable
> member in a const function, but I think it's necessary to avoid an
> end-of-stream iterator becoming valid again.
Yes, an alternative could be to reset it in increment operators but it 
had other side effects. I'll try to send you the result of this approach.
>
>> -      }
>>     return __ret;
>>       }
>>
>>       bool
>>       _M_at_eof() const
>> +      { return _S_at_eof(_M_get()); }
>> +
>> +      static bool
>> +      _S_at_eof(int_type __c)
>
> This should be called _S_is_eof, since it tests if the argument **is**
> the EOF character. It doesn't test if the stream is **at** EOF, like
> _M_at_eof does.
>
> So with a suitable ChangeLog, the "noexcept" kept on the constructor,
> and renaming _S_at_eof to _S_is_eof, this is OK for trunk
Attached patch committed with:

2017-10-04  Petr Ovtchenkov  <ptr@void-ptr.info>
         François Dumont  <fdumont@gcc.gnu.org>

     * include/bits/streambuf_iterator.h
     (istreambuf_iterator<>::operator*()): Do not capture iterator state
     in Debug assertion.
     (istreambuf_iterator<>::operator++()): Likewise and remove _M_sbuf 
check.
     (istreambuf_iterator<>::operator++(int)): Likewise.
     (istreambuf_iterator<>::_M_get()): Remove _M_c assignment.
     (istreambuf_iterator<>::_S_is_eof()): New.
     (istreambuf_iterator<>::_M_at_eof()): Adapt, use latter.
     (find(istreambuf_iterator<>, istreambuf_iterator<>, _CharT)):
     Return an iterator with _M_c set to eof to capture streambuf state
     on evaluation.
     (testsuite/24_iterators/istreambuf_iterator/2.cc): Add checks.

François

[-- Attachment #2: istreambuf_iterator.patch --]
[-- Type: text/x-patch, Size: 5057 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..64b8cfd 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,7 +95,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
       mutable streambuf_type*	_M_sbuf;
-      mutable int_type		_M_c;
+      int_type			_M_c;
 
     public:
       ///  Construct end of input stream iterator.
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       char_type
       operator*() const
       {
+	int_type __c = _M_get();
+
 #ifdef _GLIBCXX_DEBUG_PEDANTIC
 	// Dereferencing a past-the-end istreambuf_iterator is a
 	// libstdc++ extension
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(!_S_is_eof(__c),
 				_M_message(__gnu_debug::__msg_deref_istreambuf)
 				._M_iterator(*this));
 #endif
-	return traits_type::to_char_type(_M_get());
+	return traits_type::to_char_type(__c);
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf &&
+				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
-	if (_M_sbuf)
-	  {
-	    _M_sbuf->sbumpc();
-	    _M_c = traits_type::eof();
-	  }
+
+	_M_sbuf->sbumpc();
+	_M_c = traits_type::eof();
 	return *this;
       }
 
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf &&
+				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	istreambuf_iterator __old = *this;
-	if (_M_sbuf)
-	  {
-	    __old._M_c = _M_sbuf->sbumpc();
-	    _M_c = traits_type::eof();
-	  }
+	__old._M_c = _M_sbuf->sbumpc();
+	_M_c = traits_type::eof();
 	return __old;
       }
 
@@ -176,26 +175,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       int_type
       _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;
-	  }
+	int_type __ret = _M_c;
+	if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
+	  _M_sbuf = 0;
 	return __ret;
       }
 
       bool
       _M_at_eof() const
+      { return _S_is_eof(_M_get()); }
+
+      static bool
+      _S_is_eof(int_type __c)
       {
 	const int_type __eof = traits_type::eof();
-	return traits_type::eq_int_type(_M_get(), __eof);
+	return traits_type::eq_int_type(__c, __eof);
       }
     };
 
@@ -373,13 +367,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::traits_type     traits_type;
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
+      const int_type __eof = traits_type::eof();
 
       if (__first._M_sbuf && !__last._M_sbuf)
 	{
 	  const int_type __ival = traits_type::to_int_type(__val);
 	  streambuf_type* __sb = __first._M_sbuf;
 	  int_type __c = __sb->sgetc();
-	  while (!traits_type::eq_int_type(__c, traits_type::eof())
+	  while (!traits_type::eq_int_type(__c, __eof)
 		 && !traits_type::eq_int_type(__c, __ival))
 	    {
 	      streamsize __n = __sb->egptr() - __sb->gptr();
@@ -396,11 +391,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		__c = __sb->snextc();
 	    }
 
-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
-	    __first._M_c = __c;
-	  else
-	    __first._M_sbuf = 0;
+	  __first._M_c = __eof;
 	}
+
       return __first;
     }
 
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
index b81f4d4..572ea9f 100644
--- a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/2.cc
@@ -25,9 +25,7 @@
 
 void test02(void)
 {
-
   typedef std::istreambuf_iterator<char> cistreambuf_iter;
-  typedef cistreambuf_iter::streambuf_type cstreambuf_type;
   const char slit01[] = "playa hermosa, liberia, guanacaste";
   std::string str01(slit01);
   std::istringstream istrs00(str01);
@@ -35,10 +33,14 @@ void test02(void)
 
   // ctor sanity checks
   cistreambuf_iter istrb_it01(istrs00);
-  cistreambuf_iter istrb_it02;
-  std::string tmp(istrb_it01, istrb_it02); 
+  cistreambuf_iter istrb_eos;
+  VERIFY( istrb_it01 != istrb_eos );
+
+  std::string tmp(istrb_it01, istrb_eos);
   VERIFY( tmp == str01 );
 
+  VERIFY( istrb_it01 == istrb_eos );
+
   cistreambuf_iter istrb_it03(0);
   cistreambuf_iter istrb_it04;
   VERIFY( istrb_it03 == istrb_it04 );

Re: Make tests less istreambuf_iterator implementation dependent

[Jonathan Wakely]

On 04/10/17 18:21 +0200, François Dumont wrote:
>On 03/10/2017 16:20, Jonathan Wakely wrote:
>>Doesn't the modified test PASS anyway, even without changing how _M_c
>>is used?
>
>No it doesn't because the first check for eof capture the 'a' char 
>which is never reseted so after string construction it is still 
>pointing to this char and is not an eof iterator. Without the _M_c 
>assignment the iterator is still "live" and so keeps on reflecting the 
>streambuf state.

Ah yes, I must have not adjusted the test properly when I tried to
check that. Your new test does indeed fail with the old
implementation.

>Attached patch committed with:

Thanks.

[PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"

[Petr Ovtchenkov]

This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
(svn r253417).
---
 libstdc++-v3/include/bits/streambuf_iterator.h | 59 ++++++++++++++------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 081afe5..69ee013 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,7 +95,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
       mutable streambuf_type*	_M_sbuf;
-      int_type			_M_c;
+      mutable int_type		_M_c;
 
     public:
       ///  Construct end of input stream iterator.
@@ -122,29 +122,28 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       char_type
       operator*() const
       {
-	int_type __c = _M_get();
-
 #ifdef _GLIBCXX_DEBUG_PEDANTIC
 	// Dereferencing a past-the-end istreambuf_iterator is a
 	// libstdc++ extension
-	__glibcxx_requires_cond(!_S_is_eof(__c),
+	__glibcxx_requires_cond(!_M_at_eof(),
 				_M_message(__gnu_debug::__msg_deref_istreambuf)
 				._M_iterator(*this));
 #endif
-	return traits_type::to_char_type(__c);
+	return traits_type::to_char_type(_M_get());
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(!_M_at_eof(),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
-
-	_M_sbuf->sbumpc();
-	_M_c = traits_type::eof();
+	if (_M_sbuf)
+	  {
+	    _M_sbuf->sbumpc();
+	    _M_c = traits_type::eof();
+	  }
 	return *this;
       }
 
@@ -152,14 +151,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(!_M_at_eof(),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	istreambuf_iterator __old = *this;
-	__old._M_c = _M_sbuf->sbumpc();
-	_M_c = traits_type::eof();
+	if (_M_sbuf)
+	  {
+	    __old._M_c = _M_sbuf->sbumpc();
+	    _M_c = traits_type::eof();
+	  }
 	return __old;
       }
 
@@ -175,21 +176,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       int_type
       _M_get() const
       {
-	int_type __ret = _M_c;
-	if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
-	  _M_sbuf = 0;
+	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;
       }
 
       bool
       _M_at_eof() const
-      { return _S_is_eof(_M_get()); }
-
-      static bool
-      _S_is_eof(int_type __c)
       {
 	const int_type __eof = traits_type::eof();
-	return traits_type::eq_int_type(__c, __eof);
+	return traits_type::eq_int_type(_M_get(), __eof);
       }
     };
 
@@ -367,14 +373,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::traits_type     traits_type;
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
-      const int_type __eof = traits_type::eof();
 
       if (__first._M_sbuf && !__last._M_sbuf)
 	{
 	  const int_type __ival = traits_type::to_int_type(__val);
 	  streambuf_type* __sb = __first._M_sbuf;
 	  int_type __c = __sb->sgetc();
-	  while (!traits_type::eq_int_type(__c, __eof)
+	  while (!traits_type::eq_int_type(__c, traits_type::eof())
 		 && !traits_type::eq_int_type(__c, __ival))
 	    {
 	      streamsize __n = __sb->egptr() - __sb->gptr();
@@ -391,9 +396,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		__c = __sb->snextc();
 	    }
 
-	  __first._M_c = __eof;
+	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
+	    __first._M_c = __c;
+	  else
+	    __first._M_sbuf = 0;
 	}
-
       return __first;
     }
 
-- 
2.10.1

[PATCH 2/4] libstdc++: istreambuf_iterator keep attached streambuf

[Petr Ovtchenkov]

istreambuf_iterator should not forget about attached
streambuf when it reach EOF.

Checks in debug mode has no infuence more on character
extraction in istreambuf_iterator increment operators.
In this aspect behaviour in debug and non-debug mode
is similar now.

Test for detached srteambuf in istreambuf_iterator:
When istreambuf_iterator reach EOF of istream, it should not
forget about attached streambuf.
From fact "EOF in stream reached" not follow that
stream reach end of life and input operation impossible
more.

postfix increment (r++) return proxy object, due to

  copies of the previous value of r are no longer
  required either to be dereferenceable or to be in
  the domain of ==.

i.e. type that usable only for dereference and extraction
"previous" character.

istreambuf_iterator should has ctor from proxy object,
so proxy should store pointer to streambuf object.
---
 libstdc++-v3/include/bits/streambuf_iterator.h     | 67 ++++++++++++++--------
 .../24_iterators/istreambuf_iterator/3.cc          | 66 +++++++++++++++++++++
 2 files changed, 109 insertions(+), 24 deletions(-)
 create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 69ee013..08fb13b 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -98,6 +98,24 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       mutable int_type		_M_c;
 
     public:
+      class proxy
+      {
+          friend class istreambuf_iterator;
+      private:
+          proxy(int_type c, streambuf_type*	sbuf_) :
+              _M_c(c),
+              _M_sbuf(sbuf_)
+              { }
+          int_type _M_c;
+          streambuf_type*	_M_sbuf;
+
+      public:
+          char_type
+          operator*() const
+              { return traits_type::to_char_type(_M_c); }
+      };
+
+    public:
       ///  Construct end of input stream iterator.
       _GLIBCXX_CONSTEXPR istreambuf_iterator() _GLIBCXX_USE_NOEXCEPT
       : _M_sbuf(0), _M_c(traits_type::eof()) { }
@@ -116,6 +134,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
       : _M_sbuf(__s), _M_c(traits_type::eof()) { }
 
+      ///  Construct start of istreambuf iterator.
+      istreambuf_iterator(const proxy& __p) _GLIBCXX_USE_NOEXCEPT
+      : _M_sbuf(__p._M_sbuf), _M_c(traits_type::eof()) { }
+
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
       ///  operator*() on an end of stream is undefined.
@@ -136,29 +158,39 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+	__glibcxx_requires_cond(_M_sbuf,
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 	if (_M_sbuf)
 	  {
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+	    int_type __tmp =
+#endif
 	    _M_sbuf->sbumpc();
+#ifdef _GLIBCXX_DEBUG_PEDANTIC
+	    __glibcxx_requires_cond(!traits_type::eq_int_type(__tmp,traits_type::eof()),
+				    _M_message(__gnu_debug::__msg_inc_istreambuf)
+				    ._M_iterator(*this));
+#endif
 	    _M_c = traits_type::eof();
 	  }
 	return *this;
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
-      istreambuf_iterator
+      proxy
       operator++(int)
       {
-	__glibcxx_requires_cond(!_M_at_eof(),
+        _M_get();
+	__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));
 
-	istreambuf_iterator __old = *this;
+	proxy __old(_M_c, _M_sbuf);
 	if (_M_sbuf)
 	  {
-	    __old._M_c = _M_sbuf->sbumpc();
+	    _M_sbuf->sbumpc();
 	    _M_c = traits_type::eof();
 	  }
 	return __old;
@@ -177,18 +209,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;
       }
 
       bool
@@ -339,7 +362,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
 
-      if (__first._M_sbuf && !__last._M_sbuf)
+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
 	{
 	  streambuf_type* __sb = __first._M_sbuf;
 	  int_type __c = __sb->sgetc();
@@ -374,7 +397,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       typedef typename __is_iterator_type::streambuf_type  streambuf_type;
       typedef typename traits_type::int_type               int_type;
 
-      if (__first._M_sbuf && !__last._M_sbuf)
+      if (__first._M_sbuf && (__last == istreambuf_iterator<_CharT>()))
 	{
 	  const int_type __ival = traits_type::to_int_type(__val);
 	  streambuf_type* __sb = __first._M_sbuf;
@@ -395,11 +418,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	      else
 		__c = __sb->snextc();
 	    }
-
-	  if (!traits_type::eq_int_type(__c, traits_type::eof()))
-	    __first._M_c = __c;
-	  else
-	    __first._M_sbuf = 0;
+	  __first._M_c = __c;
 	}
       return __first;
     }
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
new file mode 100644
index 0000000..50413fa
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc
@@ -0,0 +1,66 @@
+// { dg-options "-std=gnu++17" }
+
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+#include <algorithm>
+#include <sstream>
+#include <iterator>
+#include <cstring>
+#include <testsuite_hooks.h>
+
+void test03()
+{
+  using namespace std;
+
+  std::stringstream s;
+  char b[] = "c2ee3d09-43b3-466d-b490-db35999a22cf";
+  char r[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
+  char q[] = "3c4852d6-d47b-4f46-b05e-b5edc1aa440e";
+  //          012345678901234567890123456789012345
+  //          0         1         2         3
+  s << b;
+  VERIFY( !s.fail() );
+
+  istreambuf_iterator<char> i(s);
+  copy_n(i, 36, r);
+  ++i; // EOF reached
+  VERIFY(i == std::istreambuf_iterator<char>());
+
+  VERIFY(memcmp(b, r, 36) == 0);
+
+  s << q;
+  VERIFY(!s.fail());
+
+  copy_n(i, 36, r);
+  ++i; // EOF reached
+  VERIFY(i == std::istreambuf_iterator<char>());
+
+  VERIFY(memcmp(q, r, 36) == 0);
+
+  s << 'Q';
+
+  VERIFY(*i++ == 'Q');
+  VERIFY(i == std::istreambuf_iterator<char>());
+}
+
+int main()
+{
+  test03();
+
+  return 0;
+}
-- 
2.10.1

[PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Petr Ovtchenkov]

Ask associated streambuf for character when needed instead of
accumulate it in istreambuf_iterator object.

Benefits from this:
  - minus one class member in istreambuf_iterator
  - trivial synchronization of states of istreambuf_iterator
    and associated streambuf
---
 libstdc++-v3/include/bits/streambuf_iterator.h | 34 ++++++++++++--------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 08fb13b..203da9d 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
       mutable streambuf_type*	_M_sbuf;
-      mutable int_type		_M_c;
 
     public:
       class proxy
       {
           friend class istreambuf_iterator;
       private:
-          proxy(int_type c, streambuf_type*	sbuf_) :
+          proxy(int_type c, streambuf_type* sbuf_) :
               _M_c(c),
               _M_sbuf(sbuf_)
               { }
           int_type _M_c;
-          streambuf_type*	_M_sbuf;
+          streambuf_type* _M_sbuf;
 
       public:
           char_type
@@ -118,7 +117,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     public:
       ///  Construct end of input stream iterator.
       _GLIBCXX_CONSTEXPR istreambuf_iterator() _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(0), _M_c(traits_type::eof()) { }
+      : _M_sbuf(0) { }
 
 #if __cplusplus >= 201103L
       istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
@@ -128,15 +127,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       ///  Construct start of input stream iterator.
       istreambuf_iterator(istream_type& __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s.rdbuf()) { }
 
       ///  Construct start of streambuf iterator.
       istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s) { }
 
       ///  Construct start of istreambuf iterator.
       istreambuf_iterator(const proxy& __p) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__p._M_sbuf), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__p._M_sbuf) { }
 
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -147,11 +146,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 #ifdef _GLIBCXX_DEBUG_PEDANTIC
 	// Dereferencing a past-the-end istreambuf_iterator is a
 	// libstdc++ extension
-	__glibcxx_requires_cond(!_M_at_eof(),
+	int_type __tmp = _M_get();
+	__glibcxx_requires_cond(!traits_type::eq_int_type(__tmp,traits_type::eof()),
 				_M_message(__gnu_debug::__msg_deref_istreambuf)
 				._M_iterator(*this));
-#endif
+	return traits_type::to_char_type(__tmp);
+#else
 	return traits_type::to_char_type(_M_get());
+#endif
       }
 
       /// Advance the iterator.  Calls streambuf.sbumpc().
@@ -172,7 +174,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 				    _M_message(__gnu_debug::__msg_inc_istreambuf)
 				    ._M_iterator(*this));
 #endif
-	    _M_c = traits_type::eof();
 	  }
 	return *this;
       }
@@ -181,17 +182,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       proxy
       operator++(int)
       {
-        _M_get();
-	__glibcxx_requires_cond(_M_sbuf
-				&& !traits_type::eq_int_type(_M_c,traits_type::eof()),
+        int_type c = _M_get();
+	__glibcxx_requires_cond(!traits_type::eq_int_type(c,traits_type::eof()),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
-	proxy __old(_M_c, _M_sbuf);
+	proxy __old(c, _M_sbuf);
 	if (_M_sbuf)
 	  {
 	    _M_sbuf->sbumpc();
-	    _M_c = traits_type::eof();
 	  }
 	return __old;
       }
@@ -209,9 +208,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       _M_get() const
       {
 	const int_type __eof = traits_type::eof();
-	if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof))
-          _M_c = _M_sbuf->sgetc();
-	return _M_c;
+	return _M_sbuf ? _M_sbuf->sgetc() : __eof;
       }
 
       bool
@@ -418,7 +415,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 	      else
 		__c = __sb->snextc();
 	    }
-	  __first._M_c = __c;
 	}
       return __first;
     }
-- 
2.10.1

[PATCH 4/4] libstdc++: immutable _M_sbuf in istreambuf_iterator

[Petr Ovtchenkov]

No needs to have mutable _M_sbuf in istreambuf_iterator
more.
---
 libstdc++-v3/include/bits/streambuf_iterator.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 203da9d..e2b6707 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,7 +94,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // the "end of stream" iterator value.
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
-      mutable streambuf_type*	_M_sbuf;
+      streambuf_type* _M_sbuf;
 
     public:
       class proxy
-- 
2.10.1

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Paolo Carlini]

Hi,

On 15/11/2017 11:48, Petr Ovtchenkov wrote:
> Ask associated streambuf for character when needed instead of
> accumulate it in istreambuf_iterator object.
>
> Benefits from this:
>    - minus one class member in istreambuf_iterator
>    - trivial synchronization of states of istreambuf_iterator
>      and associated streambuf
> ---
>   libstdc++-v3/include/bits/streambuf_iterator.h | 34 ++++++++++++--------------
>   1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
> index 08fb13b..203da9d 100644
> --- a/libstdc++-v3/include/bits/streambuf_iterator.h
> +++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> @@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>         // NB: This implementation assumes the "end of stream" value
>         // is EOF, or -1.
>         mutable streambuf_type*	_M_sbuf;
> -      mutable int_type		_M_c;
Obviously this would be an ABI-breaking change, which certainly we don't 
want. Unless I missed a detailed discussion of the non-trivial way to 
avoid it in one of the recent threads about these topics...

Paolo.

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Petr Ovtchenkov]

On Wed, 15 Nov 2017 22:31:11 +0100
Paolo Carlini <paolo.carlini@oracle.com> wrote:

> Hi,
> 
> On 15/11/2017 11:48, Petr Ovtchenkov wrote:
> > Ask associated streambuf for character when needed instead of
> > accumulate it in istreambuf_iterator object.
> >
> > Benefits from this:
> >    - minus one class member in istreambuf_iterator
> >    - trivial synchronization of states of istreambuf_iterator
> >      and associated streambuf
> > ---
> >   libstdc++-v3/include/bits/streambuf_iterator.h | 34 ++++++++++++--------------
> >   1 file changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h
> > b/libstdc++-v3/include/bits/streambuf_iterator.h index 08fb13b..203da9d 100644
> > --- a/libstdc++-v3/include/bits/streambuf_iterator.h
> > +++ b/libstdc++-v3/include/bits/streambuf_iterator.h
> > @@ -95,19 +95,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >         // NB: This implementation assumes the "end of stream" value
> >         // is EOF, or -1.
> >         mutable streambuf_type*	_M_sbuf;
> > -      mutable int_type		_M_c;
> Obviously this would be an ABI-breaking change, which certainly we don't 
> want. Unless I missed a detailed discussion of the non-trivial way to 
> avoid it in one of the recent threads about these topics...

Is we really worry about frozen sizeof of instantiated template?
(Removed private template member).

If yes, than

   int_type __dummy;

is our all.

> 
> Paolo.

--

   - ptr

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Paolo Carlini]

Hi,

On 16/11/2017 06:31, Petr Ovtchenkov wrote:
> Is we really worry about frozen sizeof of instantiated template? 
Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html 
under "Prohibited Changes", point 8.

Of course removing the buffering has performance implications too - 
that's why it's there in the first place! - which I remember we 
investigated a bit again in the past when somebody reported that a few 
implementations had it other did not. But I can't say to have followed 
all the (recently uncovered) conformance implications, it could well be 
that we cannot be 100% conforming to the letter of the current standard 
while taking advantage of a buffering mechanism. Jonathan will provide 
feedback.

Paolo.

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 10:39:02 +0100
Paolo Carlini <paolo.carlini@oracle.com> wrote:

> Hi,
> 
> On 16/11/2017 06:31, Petr Ovtchenkov wrote:
> > Is we really worry about frozen sizeof of instantiated template? 
> Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html 
> under "Prohibited Changes", point 8.
> 
> Of course removing the buffering has performance implications too - 
> that's why it's there in the first place!

"buffering" here is a secondary buffering (after streambuf).
No relation to performance, but place for incoherence with
state of attached streambuf.

Of cause, I can spend time to measure the difference.

The main point of this patch series is avoidance of lost link between
streambuf and istream_iterator when istream_iterator see eof.
Implementations that forget about attached streambuf after
istream_iterator see eof (or lost synchronization with attached
streambuf) violate principles of C++ objects life cycle.
From practical point of view, such implementation block usage of
istream_iterator for sockets, ttys, etc. --- only non-modified
files remains in scope of application.


> - which I remember we 
> investigated a bit again in the past when somebody reported that a few 
> implementations had it other did not. But I can't say to have followed 
> all the (recently uncovered) conformance implications, it could well be 
> that we cannot be 100% conforming to the letter of the current standard 
> while taking advantage of a buffering mechanism. Jonathan will provide 
> feedback.
> 
> Paolo.

--

  - ptr

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Paolo Carlini]

Hi,

On 16/11/2017 12:03, Petr Ovtchenkov wrote:
> On Thu, 16 Nov 2017 10:39:02 +0100
> Paolo Carlini <paolo.carlini@oracle.com> wrote:
>
>> Hi,
>>
>> On 16/11/2017 06:31, Petr Ovtchenkov wrote:
>>> Is we really worry about frozen sizeof of instantiated template?
>> Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
>> under "Prohibited Changes", point 8.
>>
>> Of course removing the buffering has performance implications too -
>> that's why it's there in the first place!
> "buffering" here is a secondary buffering (after streambuf).
> No relation to performance, but place for incoherence with
> state of attached streambuf.
It depends, we may be dealing with an unbuffered stream. For sure at the 
time we measured a performance impact in some cases, likewise whoever 
implemented it in the first place (not me) otherwise, again, why bothering?

Paolo.

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 12:29:37 +0100
Paolo Carlini <paolo.carlini@oracle.com> wrote:

> Hi,
> 
> On 16/11/2017 12:03, Petr Ovtchenkov wrote:
> > On Thu, 16 Nov 2017 10:39:02 +0100
> > Paolo Carlini <paolo.carlini@oracle.com> wrote:
> >
> >> Hi,
> >>
> >> On 16/11/2017 06:31, Petr Ovtchenkov wrote:
> >>> Is we really worry about frozen sizeof of instantiated template?
> >> Yes we do. See https://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
> >> under "Prohibited Changes", point 8.
> >>
> >> Of course removing the buffering has performance implications too -
> >> that's why it's there in the first place!
> > "buffering" here is a secondary buffering (after streambuf).
> > No relation to performance, but place for incoherence with
> > state of attached streambuf.
> It depends, we may be dealing with an unbuffered stream. For sure at the 
> time we measured a performance impact in some cases, likewise whoever 
> implemented it in the first place (not me) otherwise, again, why bothering?

This part of code is from SGI, so I suspect that nobody here really 
measure performance difference between "bufferred" and "non-buffered"
implementations. Just because we have only implementation
with _M_c in isreambuf_iterator.

> 
> Paolo.
> 

--

   - ptr

Re: [PATCH 3/4] libstdc++: avoid character accumulation in istreambuf_iterator

[Paolo Carlini]

Hi,

On 16/11/2017 12:41, Petr Ovtchenkov wrote:
> This part of code is from SGI, so I suspect that nobody here really
> measure performance difference between "bufferred" and "non-buffered"
> implementations.
Here where? The GNU libstdc++-v3 implementation? Certainly we did, as I 
tried to tell you the issue - the tradeoff between a more "correct", 
simpler, and certainly easier to synchronize implementation and better 
performance in some cases - isn't new. Please carry out a through search 
of Bugzilla and mailing list, there must be something recorded. I'll see 
if I can help about that, it's been a while.

Paolo.

Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"

[Jonathan Wakely]

On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
>This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
>(svn r253417).

I'm not even going to bother to review patches sent without any
explanation or rationale for the change.

I will repeat what Paolo said: changing the ABI is not acceptable.

Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 10:56:29 +0000
Jonathan Wakely <jwakely@redhat.com> wrote:

> On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
> >This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
> >(svn r253417).
> 
> I'm not even going to bother to review patches sent without any
> explanation or rationale for the change.

https://gcc.gnu.org/ml/libstdc++/2017-11/msg00044.html

Along with "violate principles of C++ objects life cycle",
the side-effect is

  - Make istreambuf_iterator::_M_sbuf immutable
  - streambuf_iterator: avoid debug-dependent behaviour

I should underline, that "_M_sbuf = 0" when istreambuf_iterator
see eof, lead to cripple lifecycle of istreambuf_iterator
object and [almost] block usage of istreambuf_iterator
for entities other then immutable files.

All tests from 24_iterators and 25_algorithms passed,
so I expect it conform to Standard.

This is series of patches, not single patch because
I keep in mind technology aspect---easy transfer
to branches other then trunk.

> 
> I will repeat what Paolo said: changing the ABI is not acceptable.

I will repeat special for you:

<snip>
Is we really worry about frozen sizeof of instantiated template?
(Removed private template member).

If yes, than

   int_type __dummy;

is our all.
</snip>

I.e. problem can be easy resolved---i.e. ABI will not suffer, if we will
reach some consensus on the main issue. 

> 

Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"

[Jonathan Wakely]

On 16/11/17 14:35 +0300, Petr Ovtchenkov wrote:
>On Thu, 16 Nov 2017 10:56:29 +0000
>Jonathan Wakely <jwakely@redhat.com> wrote:
>
>> On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
>> >This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
>> >(svn r253417).
>>
>> I'm not even going to bother to review patches sent without any
>> explanation or rationale for the change.
>
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00044.html
>
>Along with "violate principles of C++ objects life cycle",
>the side-effect is
>
>  - Make istreambuf_iterator::_M_sbuf immutable
>  - streambuf_iterator: avoid debug-dependent behaviour
>
>I should underline, that "_M_sbuf = 0" when istreambuf_iterator
>see eof, lead to cripple lifecycle of istreambuf_iterator
>object and [almost] block usage of istreambuf_iterator
>for entities other then immutable files.
>
>All tests from 24_iterators and 25_algorithms passed,
>so I expect it conform to Standard.
>
>This is series of patches, not single patch because
>I keep in mind technology aspect---easy transfer
>to branches other then trunk.
>
>>
>> I will repeat what Paolo said: changing the ABI is not acceptable.
>
>I will repeat special for you:
>
><snip>
>Is we really worry about frozen sizeof of instantiated template?
>(Removed private template member).
>
>If yes, than
>
>   int_type __dummy;
>
>is our all.
></snip>
>
>I.e. problem can be easy resolved---i.e. ABI will not suffer, if we will

What about other translation units which have inlined the old
definition of the template, and expect to find a buffered character in
that member?

>reach some consensus on the main issue.

We don't have any consensus, in fact I don't see anybody agreeing with
you, and I've previously stated I don't want to support your use case:
https://gcc.gnu.org/ml/libstdc++/2017-09/msg00100.html

Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"

[Jonathan Wakely]

On 16/11/17 11:39 +0000, Jonathan Wakely wrote:
>On 16/11/17 14:35 +0300, Petr Ovtchenkov wrote:
>>On Thu, 16 Nov 2017 10:56:29 +0000
>>Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>>>On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
>>>>This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
>>>>(svn r253417).
>>>
>>>I'm not even going to bother to review patches sent without any
>>>explanation or rationale for the change.
>>
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00044.html
>>
>>Along with "violate principles of C++ objects life cycle",
>>the side-effect is
>>
>> - Make istreambuf_iterator::_M_sbuf immutable
>> - streambuf_iterator: avoid debug-dependent behaviour
>>
>>I should underline, that "_M_sbuf = 0" when istreambuf_iterator
>>see eof, lead to cripple lifecycle of istreambuf_iterator
>>object and [almost] block usage of istreambuf_iterator
>>for entities other then immutable files.
>>
>>All tests from 24_iterators and 25_algorithms passed,
>>so I expect it conform to Standard.
>>
>>This is series of patches, not single patch because
>>I keep in mind technology aspect---easy transfer
>>to branches other then trunk.
>>
>>>
>>>I will repeat what Paolo said: changing the ABI is not acceptable.
>>
>>I will repeat special for you:
>>
>><snip>
>>Is we really worry about frozen sizeof of instantiated template?
>>(Removed private template member).
>>
>>If yes, than
>>
>>  int_type __dummy;
>>
>>is our all.
>></snip>
>>
>>I.e. problem can be easy resolved---i.e. ABI will not suffer, if we will
>
>What about other translation units which have inlined the old
>definition of the template, and expect to find a buffered character in
>that member?

In other words, the ABI is not just the "frozen sizeof".

>>reach some consensus on the main issue.
>
>We don't have any consensus, in fact I don't see anybody agreeing with
>you, and I've previously stated I don't want to support your use case:
>https://gcc.gnu.org/ml/libstdc++/2017-09/msg00100.html
>

Re: [PATCH 1/4] Revert "2017-10-04 Petr Ovtchenkov <ptr@void-ptr.info>"

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 11:39:30 +0000
Jonathan Wakely <jwakely@redhat.com> wrote:

> On 16/11/17 14:35 +0300, Petr Ovtchenkov wrote:
> >On Thu, 16 Nov 2017 10:56:29 +0000
> >Jonathan Wakely <jwakely@redhat.com> wrote:
> >
> >> On 10/10/17 22:55 +0300, Petr Ovtchenkov wrote:
> >> >This reverts commit 0dfbafdf338cc6899d146add5161e52efb02c067
> >> >(svn r253417).
> >>
> >> I'm not even going to bother to review patches sent without any
> >> explanation or rationale for the change.
> >
> >https://gcc.gnu.org/ml/libstdc++/2017-11/msg00044.html
> >
> >Along with "violate principles of C++ objects life cycle",
> >the side-effect is
> >
> >  - Make istreambuf_iterator::_M_sbuf immutable
> >  - streambuf_iterator: avoid debug-dependent behaviour
> >
> >I should underline, that "_M_sbuf = 0" when istreambuf_iterator
> >see eof, lead to cripple lifecycle of istreambuf_iterator
> >object and [almost] block usage of istreambuf_iterator
> >for entities other then immutable files.
> >
> >All tests from 24_iterators and 25_algorithms passed,
> >so I expect it conform to Standard.
> >
> >This is series of patches, not single patch because
> >I keep in mind technology aspect---easy transfer
> >to branches other then trunk.
> >
> >>
> >> I will repeat what Paolo said: changing the ABI is not acceptable.
> >
> >I will repeat special for you:
> >
> ><snip>
> >Is we really worry about frozen sizeof of instantiated template?
> >(Removed private template member).
> >
> >If yes, than
> >
> >   int_type __dummy;
> >
> >is our all.
> ></snip>
> >
> >I.e. problem can be easy resolved---i.e. ABI will not suffer, if we will
> 
> What about other translation units which have inlined the old
> definition of the template, and expect to find a buffered character in
> that member?

I can say that I can write

  int_type _M_c;

but you see, this is a _private_ member of template, so we should (may?) worry only
about size of object.

Just for clarification: Do you made accent on "buffered" or on "character" ("symbol" in ELF)?

> 
> >reach some consensus on the main issue.
> 
> We don't have any consensus, in fact I don't see anybody agreeing with
> you, and I've previously stated I don't want to support your use case:
> https://gcc.gnu.org/ml/libstdc++/2017-09/msg00100.html
> 

Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Hi

      Here is the last patch I will propose for istreambuf_iterator. 
This is mostly to remove the mutable keyword on _M_sbuf.

      To do so I had to reset _M_sbuf in valid places that is to say 
constructors and increment operators. Despite that we might still have 
eof iterators with _M_sbuf not null when you have for instance several 
iterator instance but only increment one. It seems fine to me because 
even in this case iterator will still be considered as eof and using 
several istreambuf_iterator to go through a given streambuf is not usual.

      As _M_sbuf is immutable I have been able to restore the simple 
call to _M_at_eof() in the increment operators debug check.

Ok to commit after successful tests ?

François




[-- Attachment #2: immutable_streambuf_ite.patch --]
[-- Type: text/x-patch, Size: 5861 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 081afe5..ef22aa9 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,8 +94,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // the "end of stream" iterator value.
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
-      mutable streambuf_type*	_M_sbuf;
-      int_type			_M_c;
+      streambuf_type*	_M_sbuf;
+      int_type		_M_c;
 
     public:
       ///  Construct end of input stream iterator.
@@ -110,11 +110,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       ///  Construct start of input stream iterator.
       istreambuf_iterator(istream_type& __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof())
+      { _M_init(); }
 
       ///  Construct start of streambuf iterator.
       istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s), _M_c(traits_type::eof())
+      { _M_init(); }
 
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -138,13 +140,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(!_M_at_eof(),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	_M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
+
+	if (_S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = 0;
+
 	return *this;
       }
 
@@ -152,14 +157,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(!_M_at_eof(),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	istreambuf_iterator __old = *this;
 	__old._M_c = _M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
+
+	if (_S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = __old._M_sbuf = 0;
+
 	return __old;
       }
 
@@ -172,12 +180,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return _M_at_eof() == __b._M_at_eof(); }
 
     private:
+      void
+      _M_init()
+      {
+	if (_M_sbuf && _S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = 0;
+      }
+
       int_type
       _M_get() const
       {
 	int_type __ret = _M_c;
-	if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
-	  _M_sbuf = 0;
+	if (_M_sbuf && __builtin_expect(_S_is_eof(__ret), true))
+	  __ret = _M_sbuf->sgetc();
+
 	return __ret;
       }
 
@@ -391,10 +407,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		__c = __sb->snextc();
 	    }
 
-	  __first._M_c = __eof;
+	  if (!traits_type::eq_int_type(__c, __eof))
+	    {
+	      __first._M_c = __eof;
+	      return __first;
+	    }
 	}
 
-      return __first;
+      return __last;
     }
 
 // @} group iterators
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
new file mode 100644
index 0000000..241fc58
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter eof;
+  ++eof; // Invalid.
+}
+
+int main()
+{
+  test01();
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
new file mode 100644
index 0000000..407f00b
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter eof;
+  eof++; // Invalid.
+}
+
+int main()
+{
+  test01();
+  return 0;
+}

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

Hi

      I completed execution of all tests and found one test impacted by 
this patch.

      It is a good example of the impact of the patch. Users won't be 
able to build a istreambuf_iterator at a point where the underlying 
streambuf is at end-of-stream and then put some data in the streambuf 
and then use the iterator. This is similar to what Petr was proposing, 
some eof iterator becoming valid again through an operation on the 
streambuf. I would prefer we forbid it completely or we accept it 
completely but current middle way situation is strange.

      The fix is easy, let the compiler build the streambuf_iterator 
when needed. Even if patch is not accepted I think we should keep the 
change on the test which is fragile.

François


On 13/10/2017 19:14, François Dumont wrote:
> Hi
>
>      Here is the last patch I will propose for istreambuf_iterator. 
> This is mostly to remove the mutable keyword on _M_sbuf.
>
>      To do so I had to reset _M_sbuf in valid places that is to say 
> constructors and increment operators. Despite that we might still have 
> eof iterators with _M_sbuf not null when you have for instance several 
> iterator instance but only increment one. It seems fine to me because 
> even in this case iterator will still be considered as eof and using 
> several istreambuf_iterator to go through a given streambuf is not usual.
>
>      As _M_sbuf is immutable I have been able to restore the simple 
> call to _M_at_eof() in the increment operators debug check.
>
> Ok to commit after successful tests ?
>
> François
>
>
>



[-- Attachment #2: immutable_streambuf_ite.patch --]
[-- Type: text/x-patch, Size: 9704 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 081afe5..0a6c7f9 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,7 +94,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       // the "end of stream" iterator value.
       // NB: This implementation assumes the "end of stream" value
       // is EOF, or -1.
-      mutable streambuf_type*	_M_sbuf;
+      streambuf_type*	_M_sbuf;
       int_type		_M_c;
 
     public:
@@ -110,11 +110,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       ///  Construct start of input stream iterator.
       istreambuf_iterator(istream_type& __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s.rdbuf()), _M_c(traits_type::eof())
+      { _M_init(); }
 
       ///  Construct start of streambuf iterator.
       istreambuf_iterator(streambuf_type* __s) _GLIBCXX_USE_NOEXCEPT
-      : _M_sbuf(__s), _M_c(traits_type::eof()) { }
+      : _M_sbuf(__s), _M_c(traits_type::eof())
+      { _M_init(); }
 
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -138,13 +140,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator&
       operator++()
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(!_M_at_eof(),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	_M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
+
+	if (_S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = 0;
+
 	return *this;
       }
 
@@ -152,14 +157,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(!_M_at_eof(),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	istreambuf_iterator __old = *this;
 	__old._M_c = _M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
+
+	if (_S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = __old._M_sbuf = 0;
+
 	return __old;
       }
 
@@ -172,12 +180,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return _M_at_eof() == __b._M_at_eof(); }
 
     private:
+      void
+      _M_init()
+      {
+	if (_M_sbuf && _S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = 0;
+      }
+
       int_type
       _M_get() const
       {
 	int_type __ret = _M_c;
-	if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
-	  _M_sbuf = 0;
+	if (_M_sbuf && __builtin_expect(_S_is_eof(__ret), true))
+	  __ret = _M_sbuf->sgetc();
+
 	return __ret;
       }
 
@@ -391,10 +407,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		__c = __sb->snextc();
 	    }
 
+	  if (!traits_type::eq_int_type(__c, __eof))
+	    {
 	      __first._M_c = __eof;
+	      return __first;
+	    }
 	}
 
-      return __first;
+      return __last;
     }
 
 // @} group iterators
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
index 9b69956..476e38f 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
@@ -41,7 +41,6 @@ int main()
     = std::use_facet<std::money_get<char> >(liffey.getloc());
 
   typedef std::istreambuf_iterator<char> iterator_type;
-  iterator_type is(liffey);
   iterator_type end;
 
   std::ios_base::iostate err01 = std::ios_base::goodbit;
@@ -50,7 +49,7 @@ int main()
 
   // Feed it 1 digit too many, which should fail.
   liffey.str("12.3456");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! (err01 & std::ios_base::failbit ))
     fails |= 0x01;
 
@@ -58,7 +57,7 @@ int main()
 
   // Feed it exactly what it wants, which should succeed.
   liffey.str("12.345");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if ( err01 & std::ios_base::failbit )
     fails |= 0x02;
 
@@ -66,7 +65,7 @@ int main()
 
   // Feed it 1 digit too few, which should fail.
   liffey.str("12.34");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! ( err01 & std::ios_base::failbit ))
     fails |= 0x04;
 
@@ -74,7 +73,7 @@ int main()
 
   // Feed it only a decimal-point, which should fail.
   liffey.str("12.");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! (err01 & std::ios_base::failbit ))
     fails |= 0x08;
 
@@ -82,7 +81,7 @@ int main()
 
   // Feed it no decimal-point at all, which should succeed.
   liffey.str("12");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if ( err01 & std::ios_base::failbit )
     fails |= 0x10;
 
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
index a08a713..e5f8def 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
@@ -41,7 +41,6 @@ int main()
     = std::use_facet<std::money_get<wchar_t> >(liffey.getloc());
 
   typedef std::istreambuf_iterator<wchar_t> iterator_type;
-  iterator_type is(liffey);
   iterator_type end;
 
   std::ios_base::iostate err01 = std::ios_base::goodbit;
@@ -50,7 +49,7 @@ int main()
 
   // Feed it 1 digit too many, which should fail.
   liffey.str(L"12.3456");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! (err01 & std::ios_base::failbit ))
     fails |= 0x01;
 
@@ -58,7 +57,7 @@ int main()
 
   // Feed it exactly what it wants, which should succeed.
   liffey.str(L"12.345");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if ( err01 & std::ios_base::failbit )
     fails |= 0x02;
 
@@ -66,7 +65,7 @@ int main()
 
   // Feed it 1 digit too few, which should fail.
   liffey.str(L"12.34");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! ( err01 & std::ios_base::failbit ))
     fails |= 0x04;
 
@@ -74,7 +73,7 @@ int main()
 
   // Feed it only a decimal-point, which should fail.
   liffey.str(L"12.");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! (err01 & std::ios_base::failbit ))
     fails |= 0x08;
 
@@ -82,7 +81,7 @@ int main()
 
   // Feed it no decimal-point at all, which should succeed.
   liffey.str(L"12");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if ( err01 & std::ios_base::failbit )
     fails |= 0x10;
 
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
new file mode 100644
index 0000000..241fc58
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter eof;
+  ++eof; // Invalid.
+}
+
+int main()
+{
+  test01();
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
new file mode 100644
index 0000000..407f00b
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter eof;
+  eof++; // Invalid.
+}
+
+int main()
+{
+  test01();
+  return 0;
+}

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

Hi

     Any final decision regarding this patch ?

François


On 23/10/2017 21:08, François Dumont wrote:
> Hi
>
>      I completed execution of all tests and found one test impacted by 
> this patch.
>
>      It is a good example of the impact of the patch. Users won't be 
> able to build a istreambuf_iterator at a point where the underlying 
> streambuf is at end-of-stream and then put some data in the streambuf 
> and then use the iterator. This is similar to what Petr was proposing, 
> some eof iterator becoming valid again through an operation on the 
> streambuf. I would prefer we forbid it completely or we accept it 
> completely but current middle way situation is strange.
>
>      The fix is easy, let the compiler build the streambuf_iterator 
> when needed. Even if patch is not accepted I think we should keep the 
> change on the test which is fragile.
>
> François
>
>
> On 13/10/2017 19:14, François Dumont wrote:
>> Hi
>>
>>      Here is the last patch I will propose for istreambuf_iterator. 
>> This is mostly to remove the mutable keyword on _M_sbuf.
>>
>>      To do so I had to reset _M_sbuf in valid places that is to say 
>> constructors and increment operators. Despite that we might still 
>> have eof iterators with _M_sbuf not null when you have for instance 
>> several iterator instance but only increment one. It seems fine to me 
>> because even in this case iterator will still be considered as eof 
>> and using several istreambuf_iterator to go through a given streambuf 
>> is not usual.
>>
>>      As _M_sbuf is immutable I have been able to restore the simple 
>> call to _M_at_eof() in the increment operators debug check.
>>
>> Ok to commit after successful tests ?
>>
>> François
>>
>>
>>
>
>

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Petr Ovtchenkov]

On Mon, 6 Nov 2017 22:19:22 +0100
François Dumont <frs.dumont@gmail.com> wrote:

> Hi
> 
>      Any final decision regarding this patch ?
> 
> François

https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html

--

   - ptr

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Jonathan Wakely]

On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
>On Mon, 6 Nov 2017 22:19:22 +0100
>François Dumont <frs.dumont@gmail.com> wrote:
>
>> Hi
>>
>>      Any final decision regarding this patch ?
>>
>> François
>
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html

It would be helpful if you two could collaborate and come up with a
good solution, or at least discuss the pros and cons, instead of just
sending competing patches.

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Jonathan Wakely]

On 16/11/17 10:57 +0000, Jonathan Wakely wrote:
>On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
>>On Mon, 6 Nov 2017 22:19:22 +0100
>>François Dumont <frs.dumont@gmail.com> wrote:
>>
>>>Hi
>>>
>>>     Any final decision regarding this patch ?
>>>
>>>François
>>
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
>>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
>
>It would be helpful if you two could collaborate and come up with a
>good solution, or at least discuss the pros and cons, instead of just
>sending competing patches.


Let me be more clear: I'm not going to review further patches in this
area while you two are proposing different alternatives, without
commenting on each other's approach.

If you think your solution is better than François's solution, you
should explain why, not just send a different patch. If François
thinks his solution is better than yours, he should state why, not
just send a different patch.

I don't have time to infer all that from just your patches, so I'm not
going to bother.

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 11:46:48 +0000
Jonathan Wakely <jwakely@redhat.com> wrote:

> On 16/11/17 10:57 +0000, Jonathan Wakely wrote:
> >On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
> >>On Mon, 6 Nov 2017 22:19:22 +0100
> >>François Dumont <frs.dumont@gmail.com> wrote:
> >>
> >>>Hi
> >>>
> >>>     Any final decision regarding this patch ?
> >>>
> >>>François
> >>
> >>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
> >>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
> >>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
> >>https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
> >
> >It would be helpful if you two could collaborate and come up with a
> >good solution, or at least discuss the pros and cons, instead of just
> >sending competing patches.
> 
> 
> Let me be more clear: I'm not going to review further patches in this
> area while you two are proposing different alternatives, without
> commenting on each other's approach.
> 
> If you think your solution is better than François's solution, you
> should explain why, not just send a different patch. If François
> thinks his solution is better than yours, he should state why, not
> just send a different patch.
> 
> I don't have time to infer all that from just your patches, so I'm not
> going to bother.
> 

References here is a notification that

   - there is another opinion;
   - discussion is in another thread.

Nothing more.

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

On 16/11/2017 12:46, Jonathan Wakely wrote:
> On 16/11/17 10:57 +0000, Jonathan Wakely wrote:
>> On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
>>> On Mon, 6 Nov 2017 22:19:22 +0100
>>> François Dumont <frs.dumont@gmail.com> wrote:
>>>
>>>> Hi
>>>>
>>>>     Any final decision regarding this patch ?
>>>>
>>>> François
>>>
>>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
>>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
>>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
>>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
>>
>> It would be helpful if you two could collaborate and come up with a
>> good solution, or at least discuss the pros and cons, instead of just
>> sending competing patches.
>
>
> Let me be more clear: I'm not going to review further patches in this
> area while you two are proposing different alternatives, without
> commenting on each other's approach.
>
> If you think your solution is better than François's solution, you
> should explain why, not just send a different patch. If François
> thinks his solution is better than yours, he should state why, not
> just send a different patch.
>
> I don't have time to infer all that from just your patches, so I'm not
> going to bother.
>
>
Proposing to revert my patch doesn't sound to me like a friendly action 
to start a collaboration.

My only concern has always been the Debug mode impact which is now fixed.

I already said that I disagree with Petr's main goal to keep eof 
iterator linked to the underlying stream. So current implementation is 
just fine to me and I'll let Petr argument for any change. @Jonathan, 
You can ignore my last request to remove mutable keywork on _M_sbuf.

François

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[Petr Ovtchenkov]

On Thu, 16 Nov 2017 18:40:08 +0100
François Dumont <frs.dumont@gmail.com> wrote:

> On 16/11/2017 12:46, Jonathan Wakely wrote:
> > On 16/11/17 10:57 +0000, Jonathan Wakely wrote:
> >> On 16/11/17 08:51 +0300, Petr Ovtchenkov wrote:
> >>> On Mon, 6 Nov 2017 22:19:22 +0100
> >>> François Dumont <frs.dumont@gmail.com> wrote:
> >>>
> >>>> Hi
> >>>>
> >>>>     Any final decision regarding this patch ?
> >>>>
> >>>> François
> >>>
> >>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00036.html
> >>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00035.html
> >>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
> >>> https://gcc.gnu.org/ml/libstdc++/2017-11/msg00034.html
> >>
> >> It would be helpful if you two could collaborate and come up with a
> >> good solution, or at least discuss the pros and cons, instead of just
> >> sending competing patches.
> >
> >
> > Let me be more clear: I'm not going to review further patches in this
> > area while you two are proposing different alternatives, without
> > commenting on each other's approach.
> >
> > If you think your solution is better than François's solution, you
> > should explain why, not just send a different patch. If François
> > thinks his solution is better than yours, he should state why, not
> > just send a different patch.
> >
> > I don't have time to infer all that from just your patches, so I'm not
> > going to bother.
> >
> >
> Proposing to revert my patch doesn't sound to me like a friendly action 
> to start a collaboration.

I'm already say that this is technical issue: this patch present only in
trunk yet. Series is more useful for applying in different branches.
BTW, https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
was inspired by you https://gcc.gnu.org/ml/libstdc++/2017-10/msg00029.html

> 
> My only concern has always been the Debug mode impact which is now fixed.

I hope I'm suggest identical behaviour for Debug and non-Debug mode
(no difference in interaction with associated streambuf).

> 
> I already said that I disagree with Petr's main goal to keep eof 
> iterator linked to the underlying stream.

Ok.

> So current implementation is 
> just fine to me and I'll let Petr argument for any change.

Please, clear for me: what is the "current implementation"?
Is it what we see now in trunk?

> @Jonathan, 
> You can ignore my last request to remove mutable keywork on _M_sbuf.
> 
> François
> 
> 

--

   - ptr

Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

[François Dumont]

On 16/11/2017 19:12, Petr Ovtchenkov wrote:
> On Thu, 16 Nov 2017 18:40:08 +0100
> François Dumont <frs.dumont@gmail.com> wrote:
>
>> On 16/11/2017 12:46, Jonathan Wakely wrote:
>>
>>> Let me be more clear: I'm not going to review further patches in this
>>> area while you two are proposing different alternatives, without
>>> commenting on each other's approach.
>>>
>>> If you think your solution is better than François's solution, you
>>> should explain why, not just send a different patch. If François
>>> thinks his solution is better than yours, he should state why, not
>>> just send a different patch.
>>>
>>> I don't have time to infer all that from just your patches, so I'm not
>>> going to bother.
>>>
>>>
>> Proposing to revert my patch doesn't sound to me like a friendly action
>> to start a collaboration.
> I'm already say that this is technical issue: this patch present only in
> trunk yet.
Doesn't explain why you want to revert it.
>   Series is more useful for applying in different branches.
Which branches ? There are mostly maintenance branches. None of your 
patches is fixing a bug so it won't go to a maintenance branch.
> BTW, https://gcc.gnu.org/ml/libstdc++/2017-11/msg00037.html
> was inspired by you https://gcc.gnu.org/ml/libstdc++/2017-10/msg00029.html
Which was already rejected, why submitting it again ?
>
>> So current implementation is
>> just fine to me and I'll let Petr argument for any change.
> Please, clear for me: what is the "current implementation"?
> Is it what we see now in trunk?
Yes. Other proposals were mostly code quality changes. None of them had 
no impact so Jonathan decision to keep current implementation is just fine.