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

["François Dumont" <frs.dumont@gmail.com>]

[-- 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;
+}