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

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

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

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