Re: Make istreambuf_iterator::_M_sbuf immutable and add debug checks

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

Hi

     I completed execution of all tests and found one test impacted by 
this patch.

     It is a good example of the impact of the patch. Users won't be 
able to build a istreambuf_iterator at a point where the underlying 
streambuf is at end-of-stream and then put some data in the streambuf 
and then use the iterator. This is similar to what Petr was proposing, 
some eof iterator becoming valid again through an operation on the 
streambuf. I would prefer we forbid it completely or we accept it 
completely but current middle way situation is strange.

     The fix is easy, let the compiler build the streambuf_iterator 
when needed. Even if patch is not accepted I think we should keep the 
change on the test which is fragile.

François


On 13/10/2017 19:14, François Dumont wrote:
> Hi
>
>      Here is the last patch I will propose for istreambuf_iterator. 
> This is mostly to remove the mutable keyword on _M_sbuf.
>
>      To do so I had to reset _M_sbuf in valid places that is to say 
> constructors and increment operators. Despite that we might still have 
> eof iterators with _M_sbuf not null when you have for instance several 
> iterator instance but only increment one. It seems fine to me because 
> even in this case iterator will still be considered as eof and using 
> several istreambuf_iterator to go through a given streambuf is not usual.
>
>      As _M_sbuf is immutable I have been able to restore the simple 
> call to _M_at_eof() in the increment operators debug check.
>
> Ok to commit after successful tests ?
>
> François


diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 081afe5..0a6c7f9 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -94,7 +94,7 @@ _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;
+      streambuf_type*	_M_sbuf;
       int_type		_M_c;
 
     public:
@@ -110,11 +110,13 @@ _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()), _M_c(traits_type::eof())
+      { _M_init(); }
 
       ///  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())
+      { _M_init(); }
 
       ///  Return the current character pointed to by iterator.  This returns
       ///  streambuf.sgetc().  It cannot be assigned.  NB: The result of
@@ -138,13 +140,16 @@ _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_at_eof(),
 				_M_message(__gnu_debug::__msg_inc_istreambuf)
 				._M_iterator(*this));
 
 	_M_sbuf->sbumpc();
 	_M_c = traits_type::eof();
+
+	if (_S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = 0;
+
 	return *this;
       }
 
@@ -152,14 +157,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       istreambuf_iterator
       operator++(int)
       {
-	__glibcxx_requires_cond(_M_sbuf &&
-				(!_S_is_eof(_M_c) || !_S_is_eof(_M_sbuf->sgetc())),
+	__glibcxx_requires_cond(!_M_at_eof(),
 				_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();
+
+	if (_S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = __old._M_sbuf = 0;
+
 	return __old;
       }
 
@@ -172,12 +180,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       { return _M_at_eof() == __b._M_at_eof(); }
 
     private:
+      void
+      _M_init()
+      {
+	if (_M_sbuf && _S_is_eof(_M_sbuf->sgetc()))
+	  _M_sbuf = 0;
+      }
+
       int_type
       _M_get() const
       {
 	int_type __ret = _M_c;
-	if (_M_sbuf && _S_is_eof(__ret) && _S_is_eof(__ret = _M_sbuf->sgetc()))
-	  _M_sbuf = 0;
+	if (_M_sbuf && __builtin_expect(_S_is_eof(__ret), true))
+	  __ret = _M_sbuf->sgetc();
+
 	return __ret;
       }
 
@@ -391,10 +407,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 		__c = __sb->snextc();
 	    }
 
+	  if (!traits_type::eq_int_type(__c, __eof))
+	    {
 	      __first._M_c = __eof;
+	      return __first;
+	    }
 	}
 
-      return __first;
+      return __last;
     }
 
 // @} group iterators
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
index 9b69956..476e38f 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/char/9.cc
@@ -41,7 +41,6 @@ int main()
     = std::use_facet<std::money_get<char> >(liffey.getloc());
 
   typedef std::istreambuf_iterator<char> iterator_type;
-  iterator_type is(liffey);
   iterator_type end;
 
   std::ios_base::iostate err01 = std::ios_base::goodbit;
@@ -50,7 +49,7 @@ int main()
 
   // Feed it 1 digit too many, which should fail.
   liffey.str("12.3456");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! (err01 & std::ios_base::failbit ))
     fails |= 0x01;
 
@@ -58,7 +57,7 @@ int main()
 
   // Feed it exactly what it wants, which should succeed.
   liffey.str("12.345");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if ( err01 & std::ios_base::failbit )
     fails |= 0x02;
 
@@ -66,7 +65,7 @@ int main()
 
   // Feed it 1 digit too few, which should fail.
   liffey.str("12.34");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! ( err01 & std::ios_base::failbit ))
     fails |= 0x04;
 
@@ -74,7 +73,7 @@ int main()
 
   // Feed it only a decimal-point, which should fail.
   liffey.str("12.");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! (err01 & std::ios_base::failbit ))
     fails |= 0x08;
 
@@ -82,7 +81,7 @@ int main()
 
   // Feed it no decimal-point at all, which should succeed.
   liffey.str("12");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if ( err01 & std::ios_base::failbit )
     fails |= 0x10;
 
diff --git a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
index a08a713..e5f8def 100644
--- a/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
+++ b/libstdc++-v3/testsuite/22_locale/money_get/get/wchar_t/9.cc
@@ -41,7 +41,6 @@ int main()
     = std::use_facet<std::money_get<wchar_t> >(liffey.getloc());
 
   typedef std::istreambuf_iterator<wchar_t> iterator_type;
-  iterator_type is(liffey);
   iterator_type end;
 
   std::ios_base::iostate err01 = std::ios_base::goodbit;
@@ -50,7 +49,7 @@ int main()
 
   // Feed it 1 digit too many, which should fail.
   liffey.str(L"12.3456");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! (err01 & std::ios_base::failbit ))
     fails |= 0x01;
 
@@ -58,7 +57,7 @@ int main()
 
   // Feed it exactly what it wants, which should succeed.
   liffey.str(L"12.345");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if ( err01 & std::ios_base::failbit )
     fails |= 0x02;
 
@@ -66,7 +65,7 @@ int main()
 
   // Feed it 1 digit too few, which should fail.
   liffey.str(L"12.34");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! ( err01 & std::ios_base::failbit ))
     fails |= 0x04;
 
@@ -74,7 +73,7 @@ int main()
 
   // Feed it only a decimal-point, which should fail.
   liffey.str(L"12.");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if (! (err01 & std::ios_base::failbit ))
     fails |= 0x08;
 
@@ -82,7 +81,7 @@ int main()
 
   // Feed it no decimal-point at all, which should succeed.
   liffey.str(L"12");
-  greed.get(is, end, false, liffey, err01, coins);
+  greed.get(liffey, end, false, liffey, err01, coins);
   if ( err01 & std::ios_base::failbit )
     fails |= 0x10;
 
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
new file mode 100644
index 0000000..241fc58
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/1_neg.cc
@@ -0,0 +1,35 @@
+// 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/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter eof;
+  ++eof; // Invalid.
+}
+
+int main()
+{
+  test01();
+  return 0;
+}
diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
new file mode 100644
index 0000000..407f00b
--- /dev/null
+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/debug/2_neg.cc
@@ -0,0 +1,35 @@
+// 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/>.
+
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+
+void test01()
+{
+  typedef std::istreambuf_iterator<char> cistreambuf_iter;
+
+  cistreambuf_iter eof;
+  eof++; // Invalid.
+}
+
+int main()
+{
+  test01();
+  return 0;
+}