This is the mail archive of the libstdc++@gcc.gnu.org mailing list for the libstdc++ project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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




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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]