Re: Add std::copy_n overload for istreambuf_iterator

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On Thu, 10 Oct 2019 at 21:57, François Dumont <frs.dumont@gmail.com> wrote:

> I think this build has been done between the 2 commits I used to apply
> this patch (because of a problem between the chair and the keyboard). Now
> it should be fine.
>
Indeed the test passes since at least r 276644
Thanks


> But it shows that it changed the behavior of std::copy_n as soon as the
> new specialization is being used.
>
> Without this change the result is "12234" whereas with the specialization
> it is "12345". So in addition to being a nice perf enhancement this patch
> was also a partial fix for PR 81857.
>
> Let me know Jonathan if there is something to do about it.
>
> François
>
> On 10/9/19 10:18 PM, Christophe Lyon wrote:
>
>
>
> On Fri, 4 Oct 2019 at 07:01, François Dumont <frs.dumont@gmail.com> wrote:
>
>> On 9/27/19 1:00 PM, Jonathan Wakely wrote:
>> > On 19/07/19 23:37 +0200, François Dumont wrote:
>> >> It sounds reasonable to overload std::copy_n for istreambuf_iterator.
>> > I wonder whether it's worth doing:
>> >
>> > #if __cplusplus >= 201703L
>> >    if constexpr (is_same_v<_OutputIterator, _CharT*>)
>> >      return __result + __it._M_sbuf->sgetn(__result, __n);
>> >    else
>> >      {
>> > #endif
>> >      ...
>> > #if __cplusplus >= 201703L
>> >      }
>> > #endif
>> >
>> > We could extend that to also work for basic_string<_CharT>::iterator
>> > and vector<_CharT>::iterator too if we wanted.
>> >
>> > I'm not sure if it will perform any better than the code below (it's
>> > approximately equivalent) but it should result in smaller binaries, as
>> we
>> > wouldn't be instantiating the code below when outputting to a pointer
>> > or contiguous iterator.
>> >
>> > We don't need to do that now, it can be a separate patch later (if we
>> > do it at all).
>>
>> Very good remark, I hadn't check streambuf to find out if there were
>> better to do. For me it is the streambuf method to target for an
>> std::copy_n overload.
>>
>> So here is a new proposal much simpler. I see no reason to enable it
>> only for char types, is there ?
>>
>> Once the other patch on copy/copy_backward... algos is in I'll provide
>> what necessary to benefit from the same optimization for std::deque
>> iterators and in Debug mode.
>>
>> >
>> >> +#endif
>> >
>> > Because the matching #if is more than 40 lines away, please add a
>> > comment noting the condition that this corresponds to, i.e.
>> >
>> > #endif // C++11
>> Ok, done even if there is no 40 lines anymore. And also added it in
>> stl_algo.h.
>> >
>> >> +
>> >>   template<typename _CharT>
>> >>     typename __gnu_cxx::__enable_if<__is_char<_CharT>::__value,
>> >>                       istreambuf_iterator<_CharT> >::__type
>> >> diff --git a/libstdc++-v3/include/std/streambuf
>> >> b/libstdc++-v3/include/std/streambuf
>> >> index d9ca981d704..4f62ebf4d95 100644
>> >> --- a/libstdc++-v3/include/std/streambuf
>> >> +++ b/libstdc++-v3/include/std/streambuf
>> >> @@ -155,6 +155,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>> >>         __copy_move_a2(istreambuf_iterator<_CharT2>,
>> >>                istreambuf_iterator<_CharT2>, _CharT2*);
>> >>
>> >> +#if __cplusplus >= 201103L
>> >> +      template<typename _CharT2, typename _Size, typename
>> >> _OutputIterator>
>> >> +    friend typename enable_if<__is_char<_CharT2>::__value,
>> >> +                  _OutputIterator>::type
>> >> +    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>> >> +#endif
>> >> +
>> >>       template<typename _CharT2>
>> >>         friend typename
>> >> __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
>> >>                   istreambuf_iterator<_CharT2> >::__type
>> >> diff --git
>> >> a/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>> >> b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>> >> new file mode 100644
>> >> index 00000000000..ebd769cf7c0
>> >> --- /dev/null
>> >> +++
>> b/libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator.cc
>> >> @@ -0,0 +1,59 @@
>> >> +// Copyright (C) 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/>.
>> >> +
>> >> +// { dg-do run { target c++11 } }
>> >> +
>> >> +#include <algorithm>
>> >> +#include <sstream>
>> >> +#include <iterator>
>> >> +
>> >> +#include <testsuite_hooks.h>
>> >> +
>> >> +void test01()
>> >> +{
>> >> +  std::stringstream ss("12345");
>> >> +
>> >> +  std::string ostr(5, '0');
>> >> +  typedef std::istreambuf_iterator<char> istrb_ite;
>> >> +  std::copy_n(istrb_ite(ss), 0, ostr.begin());
>> >> +  VERIFY( ostr.front() == '0' );
>> >
>> > I'd like to see a check here that the value returned from copy_n is
>> > equal to ostr.begin().
>> >
>> >> +
>> >> +  std::copy_n(istrb_ite(ss), 2, ostr.begin());
>> >> +  VERIFY( ostr == "12000" );
>> >
>> > And equal to ostr.begin() + 2.
>> >
>> >> +
>> >> +  std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2);
>> >> +  VERIFY( ostr == "12345" );
>> >
>> > And equal to ostr.begin() + 5 here.
>> Done.
>> >
>> >> +}
>> >> +
>> >> +void test02()
>> >> +{
>> >> +  std::stringstream ss("12345");
>> >> +
>> >> +  std::string ostr(5, '0');
>> >> +  typedef std::istreambuf_iterator<char> istrb_ite;
>> >> +
>> >> +  istrb_ite ibfit(ss);
>> >> +  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, ostr.begin()));
>> >> +  VERIFY( ostr == "12345" );
>> >> +}
>> >>
>> >
>> > Ideally I'd also like to see tests where the input buffer is larger
>> > than the size being read, e.g. read 5 chars from "123456" and verify
>> > we don't read the '6'.
>> In test01 I am doing something like that.
>> >
>> > Also, these tests don't exercise the code path that causes an
>> > underflow. It would be good to use an ifstream to read from one of the
>> > files in the testsuite/data directory, and read a large amount of data
>> > (more than fits in a filebuf's read area) so that the underflow logic
>> > is tested.
>>
>> With this new proposal I don't need to do it, I'am counting on sgetn
>> tests.
>>
>>      * include/bits/stl_algo.h
>>      (__copy_n_a(_IIte, _Size, _OIte)): New.
>>      (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New declaration.
>>      (__copy_n(_IIte, _Size, _OIte, input_iterator_tag)): Adapt to use
>>      latter.
>>      * include/bits/streambuf_iterator.h (istreambuf_iterator<>): Declare
>>      std::__copy_n_a friend.
>>      (__copy_n_a(istreambuf_iterator<>, _Size, _CharT*)): New.
>>      * testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc: New.
>>
>
> Hi,
>
> Maybe this has already been reported, but I've noticed that this new test
> fails on arm & aarch64.
> My libstdc++.log says:
>  /libstdc++-v3/testsuite/25_algorithms/copy_n/istreambuf_iterator/1.cc:42:
> void test01(): Assertion 'ostr == "12345"' failed.
>
> Christophe
>
>      * testsuite/25_algorithms/copy_n/istreambuf_iterator/1_neg.cc: New.
>>      * testsuite/25_algorithms/copy_n/istreambuf_iterator/2_neg.cc: New.
>>
>> Tested under Linux x86_64.
>>
>> Ok to commit ?
>>
>> François
>>
>>
>