libstdc++: istreambuf_iterator keep attached streambuf

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

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.

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.

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

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.

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.

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

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

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

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

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.

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.