| Message ID | 5531cf67-62b3-a4bc-c611-0fe026b75066@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Implement std::advance for istreambuf_iterator using pubseekoff | expand |
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 >
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 > > >
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 >>>
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*);