[PATCH] Implement std::advance for istreambuf_iterator using pubseekoff

[PATCH] Implement std::advance for istreambuf_iterator using pubseekoff

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

The same way I proposed to review std::copy overload for 
istreambuf_iterator we can implement std::advance using pubseekoff.

It is both a cleaner implementation and avoids yet another friend 
declaration.


     * include/std/streambuf
     (advance(istreambuf_iterator<>&, _Distance)): Remove friend 
declaration.
     * include/bits/streambuf_iterator.h (__copy_move_a2): Re-implement 
using
     streambuf pubseekoff.

Tested under Linux x86_64.

Ok to commit ?

François


[-- Attachment #2: istreambuf_ite_advance.patch --]
[-- Type: text/x-patch, Size: 2529 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 134b3486b9a..afe5c95f021 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -434,34 +434,19 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 			      _M_message(__gnu_debug::__msg_inc_istreambuf)
 			      ._M_iterator(__i));
 
-      typedef istreambuf_iterator<_CharT>		   __is_iterator_type;
-      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();
-
-      streambuf_type* __sb = __i._M_sbuf;
-      while (__n > 0)
-	{
-	  streamsize __size = __sb->egptr() - __sb->gptr();
-	  if (__size > __n)
-	    {
-	      __sb->__safe_gbump(__n);
-	      break;
-	    }
-
-	  __sb->__safe_gbump(__size);
-	  __n -= __size;
-	  if (traits_type::eq_int_type(__sb->underflow(), __eof))
-	    {
-	      __glibcxx_requires_cond(__n == 0,
-				_M_message(__gnu_debug::__msg_inc_istreambuf)
-				._M_iterator(__i));
-	      break;
-	    }
-	}
+#ifdef _GLIBCXX_DEBUG
+      typedef basic_streambuf<_CharT> __streambuf_t;
+      typedef typename __streambuf_t::pos_type __pos_type;
+      __pos_type __cur_pos
+	= __i._M_sbuf->pubseekoff(0, ios_base::cur, ios_base::in);
+      __pos_type __new_pos =
+#endif
+      __i._M_sbuf->pubseekoff(__n, ios_base::cur, ios_base::in);
+      __i._M_c = char_traits<_CharT>::eof();
 
-      __i._M_c = __eof;
+      __glibcxx_requires_cond(__new_pos - __cur_pos == __n,
+			      _M_message(__gnu_debug::__msg_inc_istreambuf)
+			      ._M_iterator(__i));
     }
 
 // @} group iterators
diff --git a/libstdc++-v3/include/std/streambuf b/libstdc++-v3/include/std/streambuf
index 3442f19bd78..ef03da39bc2 100644
--- a/libstdc++-v3/include/std/streambuf
+++ b/libstdc++-v3/include/std/streambuf
@@ -155,11 +155,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
         find(istreambuf_iterator<_CharT2>, istreambuf_iterator<_CharT2>,
 	     const _CharT2&);
 
-      template<typename _CharT2, typename _Distance>
-        friend typename __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
-					       void>::__type
-        advance(istreambuf_iterator<_CharT2>&, _Distance);
-
       template<typename _CharT2, typename _Traits2>
         friend basic_istream<_CharT2, _Traits2>&
         operator>>(basic_istream<_CharT2, _Traits2>&, _CharT2*);

Re: [PATCH] Implement std::advance for istreambuf_iterator using pubseekoff

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

Here is an update to set _M_sbuf to null if the user advance too much.

Note that in this case the streambuf remains un-modified which is 
different from the current implementation. I think it is another 
enhancement.

I also change the Debug assertion message for something more dedicated 
to std::advance algo.

François

On 10/14/19 10:12 PM, François Dumont wrote:
> The same way I proposed to review std::copy overload for 
> istreambuf_iterator we can implement std::advance using pubseekoff.
>
> It is both a cleaner implementation and avoids yet another friend 
> declaration.
>
>
>     * include/std/streambuf
>     (advance(istreambuf_iterator<>&, _Distance)): Remove friend 
> declaration.
>     * include/bits/streambuf_iterator.h (__copy_move_a2): Re-implement 
> using
>     streambuf pubseekoff.
>
> Tested under Linux x86_64.
>
> Ok to commit ?
>
> François
>


[-- Attachment #2: istreambuf_ite_advance.patch --]
[-- Type: text/x-patch, Size: 4445 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 134b3486b9a..17dd3972d45 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -431,37 +431,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       __glibcxx_assert(__n > 0);
       __glibcxx_requires_cond(!__i._M_at_eof(),
-			      _M_message(__gnu_debug::__msg_inc_istreambuf)
-			      ._M_iterator(__i));
-
-      typedef istreambuf_iterator<_CharT>		   __is_iterator_type;
-      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();
-
-      streambuf_type* __sb = __i._M_sbuf;
-      while (__n > 0)
-	{
-	  streamsize __size = __sb->egptr() - __sb->gptr();
-	  if (__size > __n)
+			      _M_message(__gnu_debug::__msg_advance_oob)
+			      ._M_iterator(__i)
+			      ._M_integer(__n));
+
+      typedef basic_streambuf<_CharT> __streambuf_t;
+      typedef typename __streambuf_t::pos_type __pos_t;
+      __pos_t __cur_pos
+	= __i._M_sbuf->pubseekoff(0, ios_base::cur, ios_base::in);
+      __pos_t __new_pos
+	= __i._M_sbuf->pubseekoff(__n, ios_base::cur, ios_base::in);
+      __i._M_c = char_traits<_CharT>::eof();
+
+      if (__new_pos - __cur_pos != __n)
 	{
-	      __sb->__safe_gbump(__n);
-	      break;
-	    }
-
-	  __sb->__safe_gbump(__size);
-	  __n -= __size;
-	  if (traits_type::eq_int_type(__sb->underflow(), __eof))
-	    {
-	      __glibcxx_requires_cond(__n == 0,
-				_M_message(__gnu_debug::__msg_inc_istreambuf)
-				._M_iterator(__i));
-	      break;
-	    }
+	  __i._M_sbuf = 0;
+	  __glibcxx_requires_cond(!__i._M_at_eof(),
+				  _M_message(__gnu_debug::__msg_advance_oob)
+				  ._M_iterator(__i)
+				  ._M_integer(__n));
 	}
-
-      __i._M_c = __eof;
     }
 
 // @} group iterators
diff --git a/libstdc++-v3/include/std/streambuf b/libstdc++-v3/include/std/streambuf
index 3442f19bd78..ef03da39bc2 100644
--- a/libstdc++-v3/include/std/streambuf
+++ b/libstdc++-v3/include/std/streambuf
@@ -155,11 +155,6 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
         find(istreambuf_iterator<_CharT2>, istreambuf_iterator<_CharT2>,
 	     const _CharT2&);
 
-      template<typename _CharT2, typename _Distance>
-        friend typename __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
-					       void>::__type
-        advance(istreambuf_iterator<_CharT2>&, _Distance);
-
       template<typename _CharT2, typename _Traits2>
         friend basic_istream<_CharT2, _Traits2>&
         operator>>(basic_istream<_CharT2, _Traits2>&, _CharT2*);
diff --git a/libstdc++-v3/testsuite/25_algorithms/advance/istreambuf_iterators/char/3.cc b/libstdc++-v3/testsuite/25_algorithms/advance/istreambuf_iterators/char/3.cc
new file mode 100644
index 00000000000..8763e7fa78e
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/advance/istreambuf_iterators/char/3.cc
@@ -0,0 +1,49 @@
+// Copyright (C) 2017-2019 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/>.
+
+// Debug mode would detect the invalid std::advance call.
+// { dg-require-normal-mode "" }
+
+#include <iterator>
+#include <sstream>
+#include <algorithm>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  using namespace std;
+
+  typedef istreambuf_iterator<char> in_iterator_type;
+
+  const char data1[] = "Drei Phantasien nach Friedrich Holderlin";
+  istringstream iss1(data1);
+  in_iterator_type beg1(iss1);
+  in_iterator_type end1;
+
+  VERIFY( beg1 != end1 );
+
+  advance(beg1, sizeof(data1));
+
+  VERIFY( beg1 == end1 );
+}
+
+int main()
+{
+  test01();
+  return 0;
+}

Re: [PATCH] Implement std::advance for istreambuf_iterator using pubseekoff

[Jonathan Wakely]

On Tue, 15 Oct 2019 at 21:20, François Dumont wrote:
>
> Here is an update to set _M_sbuf to null if the user advance too much.
>
> Note that in this case the streambuf remains un-modified which is
> different from the current implementation. I think it is another
> enhancement.
>
> I also change the Debug assertion message for something more dedicated
> to std::advance algo.
>
> François
>
> On 10/14/19 10:12 PM, François Dumont wrote:
> > The same way I proposed to review std::copy overload for
> > istreambuf_iterator we can implement std::advance using pubseekoff.
> >
> > It is both a cleaner implementation and avoids yet another friend
> > declaration.

Looks like I never sent my review of this one, it's been sitting in my
draft mails for years, sorry.

It looks like this will fail if the streambuf doesn't support seeking.
The default behaviour for seekoff is to return -1, in which case
you'll get -1 for both calls to pubseekoff, and new_pos - cur_pos will
be zero, which is not equal to n, so you set the istreambuf_iterator
to end-of-stream. That seems wrong, we could still advance using the
old code (or just call ++ in a loop!)



> >
> >     * include/std/streambuf
> >     (advance(istreambuf_iterator<>&, _Distance)): Remove friend
> > declaration.
> >     * include/bits/streambuf_iterator.h (__copy_move_a2): Re-implement
> > using
> >     streambuf pubseekoff.
> >
> > Tested under Linux x86_64.
> >
> > Ok to commit ?
> >
> > François
> >
>

Re: [PATCH] Implement std::advance for istreambuf_iterator using pubseekoff

[François Dumont]

[-- Attachment #1: Type: text/plain, Size: 2741 bytes --]

I had totally forgotten about it myself. The occasion to take a fresh 
look at it.

Here is a new version considering 1st pubseekoff call returned value to 
find out if it can be used. I removed the check/comparison with 2nd call 
result as it's not usable. In new 4_neg.cc test case if I ask to advance 
istreambuf_iterator by 100000 the returned pos_type is showing 100000 
offset even if it is obviously false. You need to read next byte to see 
that it is eof.

     libstdc++: Implement std::advance for istreambuf_iterator using 
pubseekoff

     If advance increment is smaller than input buffer size just advance 
in this
     buffer thanks to __safe_gbump. If increment is larger check for 
seekoff support
     and use it accordingly. Otherwise fallback on current 
__safe_gbump/underflow loop.

     libstdc++-v3/ChangeLog:

             * include/bits/streambuf_iterator.h (advance): Re-implement
             using streambuf pubseekoff/seekoff when supported.
             * 
testsuite/25_algorithms/advance/istreambuf_iterators/char/4_neg.cc: New
             test case.

Ok to commit when back in stage 1 ?

François


On 31/03/2023 23:03, Jonathan Wakely wrote:
> On Tue, 15 Oct 2019 at 21:20, François Dumont wrote:
>> Here is an update to set _M_sbuf to null if the user advance too much.
>>
>> Note that in this case the streambuf remains un-modified which is
>> different from the current implementation. I think it is another
>> enhancement.
>>
>> I also change the Debug assertion message for something more dedicated
>> to std::advance algo.
>>
>> François
>>
>> On 10/14/19 10:12 PM, François Dumont wrote:
>>> The same way I proposed to review std::copy overload for
>>> istreambuf_iterator we can implement std::advance using pubseekoff.
>>>
>>> It is both a cleaner implementation and avoids yet another friend
>>> declaration.
> Looks like I never sent my review of this one, it's been sitting in my
> draft mails for years, sorry.
>
> It looks like this will fail if the streambuf doesn't support seeking.
> The default behaviour for seekoff is to return -1, in which case
> you'll get -1 for both calls to pubseekoff, and new_pos - cur_pos will
> be zero, which is not equal to n, so you set the istreambuf_iterator
> to end-of-stream. That seems wrong, we could still advance using the
> old code (or just call ++ in a loop!)
>
>
>
>>>      * include/std/streambuf
>>>      (advance(istreambuf_iterator<>&, _Distance)): Remove friend
>>> declaration.
>>>      * include/bits/streambuf_iterator.h (__copy_move_a2): Re-implement
>>> using
>>>      streambuf pubseekoff.
>>>
>>> Tested under Linux x86_64.
>>>
>>> Ok to commit ?
>>>
>>> François
>>>

[-- Attachment #2: istreambuf_iterator_advance.patch --]
[-- Type: text/x-patch, Size: 3778 bytes --]

diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index 32285a668fc..4527660e191 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -479,37 +479,59 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 
       __glibcxx_assert(__n > 0);
       __glibcxx_requires_cond(!__i._M_at_eof(),
-			      _M_message(__gnu_debug::__msg_inc_istreambuf)
-			      ._M_iterator(__i));
+			      _M_message(__gnu_debug::__msg_advance_oob)
+			      ._M_iterator(__i)
+			      ._M_integer(__n));
 
       typedef istreambuf_iterator<_CharT>		   __is_iterator_type;
       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;
+      typedef typename traits_type::off_type		   off_type;
+      typedef typename streambuf_type::pos_type		   pos_type;
       const int_type __eof = traits_type::eof();
+      const pos_type __npos = pos_type(off_type(-1));
 
       streambuf_type* __sb = __i._M_sbuf;
-      while (__n > 0)
-	{
       streamsize __size = __sb->egptr() - __sb->gptr();
-	  if (__size > __n)
-	    {
+      if (__size >= __n)
 	__sb->__safe_gbump(__n);
-	      break;
-	    }
-
+      else
+	{
+	  // Check for seekoff support.
+	  if (__sb->pubseekoff(0, ios_base::cur, ios_base::in) != __npos)
+	    __sb->pubseekoff(__n, ios_base::cur, ios_base::in);
+	  else
+	    {
+	      _GLIBCXX_DEBUG_ONLY(_Distance __n_orig = __n);
+	      for (;;)
+		{
 		  __sb->__safe_gbump(__size);
 		  __n -= __size;
+
+		  if (__n == 0)
+		    break;
+
 		  if (traits_type::eq_int_type(__sb->underflow(), __eof))
 		    {
-	      __glibcxx_requires_cond(__n == 0,
-				_M_message(__gnu_debug::__msg_inc_istreambuf)
-				._M_iterator(__i));
+		      __i._M_sbuf = 0;
 		      break;
 		    }
+
+		  __size = __sb->egptr() - __sb->gptr();
+		  if (__size > __n)
+		    __size = __n;
+		}
+
+	      _GLIBCXX_DEBUG_ONLY(__n = __n_orig);
+	    }
 	}
 
       __i._M_c = __eof;
+      __glibcxx_requires_cond(!__i._M_at_eof(),
+			      _M_message(__gnu_debug::__msg_advance_oob)
+			      ._M_iterator(__i)
+			      ._M_integer(__n));
     }
 
 /// @} group iterators
diff --git a/libstdc++-v3/testsuite/25_algorithms/advance/istreambuf_iterators/char/4_neg.cc b/libstdc++-v3/testsuite/25_algorithms/advance/istreambuf_iterators/char/4_neg.cc
new file mode 100644
index 00000000000..88ccaeff499
--- /dev/null
+++ b/libstdc++-v3/testsuite/25_algorithms/advance/istreambuf_iterators/char/4_neg.cc
@@ -0,0 +1,65 @@
+// { dg-require-fileio "" }
+// { dg-do run { xfail *-*-* } }
+// { dg-require-debug-mode "" }
+
+#include <iterator>
+#include <fstream>
+#include <algorithm>
+
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  using namespace std;
+
+  typedef istreambuf_iterator<char> in_iterator_type;
+
+  unsigned found = 0;
+
+  {
+    ifstream fbuf("istream_unformatted-1.txt");
+
+    in_iterator_type beg(fbuf);
+    in_iterator_type end;
+
+    for (;;)
+      {
+	beg = find(beg, end, '1');
+	if (beg == end)
+	  break;
+
+	++found;
+	VERIFY( *beg == '1' );
+
+	advance(beg, 9);
+	VERIFY( *beg == '0' );
+      }
+  }
+
+  {
+    ifstream fbuf("istream_unformatted-1.txt");
+
+    in_iterator_type beg(fbuf);
+    in_iterator_type end;
+
+    beg = find(beg, end, '1');
+    VERIFY( beg != end );
+    VERIFY( *beg == '1' );
+
+    advance(beg, 9);
+    VERIFY( *beg == '0' );
+
+    unsigned line_length = 10;
+    while (*++beg != '1')
+      ++line_length;
+
+    // Try to jump directly to the end + 1 through advance.
+    advance(beg, (found - 2) * line_length + 9 + 2);
+  }
+}
+
+int main()
+{
+  test01();
+  return 0;
+}