Re: Make tests less istreambuf_iterator implementation dependent

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

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

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