Re: Make tests less istreambuf_iterator implementation dependent

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

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

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