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

Completing the execution of tests revealed a lot about the current implementation.

The main point of current implementation is to delay as much as possible the capture of the current streambuf position. So my original proposal capturing state on instantiation was wrong.

This new proposal concentrate on the debug-dependent code. Debug assertions now avoids calling _M_at_eof() which also capture iterator state. It also simplifies _M_get() method a little bit like Petr proposed but keeping the _M_sbuf reset when reaching eof. Thanks to this work I realized that std::find specialization could also be simplified by returning a streambuf_iterator which will capture current streambuf state on evaluation.

Note that I haven't been able to create a test case revealing the problem. This is more a code quality issue as current code violates the principal that debug asserts shouldn't impact object state. AFAIK this is noticeable only under gdb.

Regarding avoiding the reset of _M_sbuf it might be possible,your test case could be a good reason to do so. But this is going to be a big change for current implementation so don't forget to run all testsuite and to consider the std::copy and std::find specializations.

Tested under Linux x86_64, normal and debug modes.

Ok to commit ?

François


On 08/09/2017 07:47, Petr Ovtchenkov wrote:
-gcc-patches

On Thu, 7 Sep 2017 23:02:15 +0200
François Dumont <frs.dumont@gmail.com> wrote:

+	_M_c = _M_sbuf->sgetc();
+	if (_S_at_eof(_M_c))
+	  _M_sbuf = 0;
_M_sbuf = 0; <--- Is not what I axpect here.

Suggestions will be later, after we finish copyright assignment for
changes procedure (in progress).

WBR,

--

    - ptr


diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..ad9c89f 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -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,20 @@ _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
+	if (_M_sbuf && _S_at_eof(_M_c) && _S_at_eof(_M_c = _M_sbuf->sgetc()))
 	  _M_sbuf = 0;
-	  }
-	return __ret;
+	return _M_c;
       }
 
       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 +366,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 +390,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..e3d99f9 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,17 @@ 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 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]