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

["François Dumont" <frs.dumont@gmail.com>]

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