Re: Add std::copy_n overload for istreambuf_iterator

[Christophe Lyon]

On Thu, 10 Oct 2019 at 21:57, François Dumont  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  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 __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> >> _OutputIterator>
>> >> +friend typename enable_if<__is_char<_CharT2>::__value,
>> >> +  _OutputIterator>::type
>> >> +copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>> >> +#endif
>> >> +
>> >>   template
>> >> 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 000..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
>> >> +// .
>> >> +
>> >> +// { dg-do run { target c++11 } }
>> >> +
>> >> +#include 
>> >> +#include 
>> >> +#include 
>> >> +
>> >> +#include 
>> >> +
>> >> +void test01()
>> >> +{
>> >> +  std::stringstream ss("12345");
>> >> +
>> >> +  

Re: Add std::copy_n overload for istreambuf_iterator

[François Dumont]

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.


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 > 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 __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> _OutputIterator>
>> +    friend typename enable_if<__is_char<_CharT2>::__value,
>> +  _OutputIterator>::type
>> +    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>> +#endif
>> +
>>   template
>>     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 000..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
>> +// .
>> +
>> +// { dg-do run { target c++11 } }
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +void test01()
>> +{
>> +  std::stringstream ss("12345");
>> +
>> +  std::string ostr(5, '0');
>> 

Re: Add std::copy_n overload for istreambuf_iterator

[François Dumont]

On 10/9/19 10:18 PM, Christophe Lyon wrote:



On Fri, 4 Oct 2019 at 07:01, François Dumont > 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 __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> _OutputIterator>
>> +    friend typename enable_if<__is_char<_CharT2>::__value,
>> +  _OutputIterator>::type
>> +    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
>> +#endif
>> +
>>   template
>>     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 000..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
>> +// .
>> +
>> +// { dg-do run { target c++11 } }
>> +
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include 
>> +
>> +void test01()
>> +{
>> +  std::stringstream ss("12345");
>> +
>> +  std::string ostr(5, '0');
>> +  typedef std::istreambuf_iterator 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 

Re: Add std::copy_n overload for istreambuf_iterator

[Christophe Lyon]

On Fri, 4 Oct 2019 at 07:01, François Dumont  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 __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 >> _OutputIterator>
> >> +friend typename enable_if<__is_char<_CharT2>::__value,
> >> +  _OutputIterator>::type
> >> +copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
> >> +#endif
> >> +
> >>   template
> >> 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 000..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
> >> +// .
> >> +
> >> +// { dg-do run { target c++11 } }
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +#include 
> >> +
> >> +void test01()
> >> +{
> >> +  std::stringstream ss("12345");
> >> +
> >> +  std::string ostr(5, '0');
> >> +  typedef std::istreambuf_iterator 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 istrb_ite;
> >> +
> >> +  istrb_ite ibfit(ss);
> >> +  std::copy_n(ibfit, 3, std::copy_n(ibfit, 2, 

Re: Add std::copy_n overload for istreambuf_iterator

[Jonathan Wakely]

On 04/10/19 07:01 +0200, François Dumont 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 ?


The reason to only do this for char-like types and std::char_traits is
that users could specialize istreambuf_iterator on their own types or
their own traits, and the specialization would not declare __copy_n_a
as a friend, and would not have the _M_sbuf member.

So I think we should still limit the optimisation to __is_char types
and std::char_traits. In practice that will help for all reasonable
cases, and unreasonable users who specialize the class can still
compile, but don't get the optimisation.



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.


Thanks.


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.


Agreed.

OK for trunk with the __is_char<_CharT> and char_traits<_CharT>
constraint restored. Thanks!

Re: Add std::copy_n overload for istreambuf_iterator

[François Dumont]

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 __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_OutputIterator>

+    friend typename enable_if<__is_char<_CharT2>::__value,
+  _OutputIterator>::type
+    copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
+#endif
+
  template
    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 000..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
+// .
+
+// { dg-do run { target c++11 } }
+
+#include 
+#include 
+#include 
+
+#include 
+
+void test01()
+{
+  std::stringstream ss("12345");
+
+  std::string ostr(5, '0');
+  typedef std::istreambuf_iterator 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 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.

    * 

Re: Add std::copy_n overload for istreambuf_iterator

[Jonathan Wakely]

On 19/07/19 23:37 +0200, François Dumont wrote:

It sounds reasonable to overload std::copy_n for istreambuf_iterator.

    * include/bits/stl_algo.h (copy_n(istreambuf_iterator<>, _Size, 
_OIte)):

    New declaration.
    * include/bits/streambuf_iterator.h (istreambuf_iterator<>): Declare
    std::copy_n for istreambuf_iterator of char types to be friend.
    (std::copy_n(istreambuf_iterator<>, _Size, _OIte)): New overload.
    * include/std/streambuf(basic_streambuf<>): Declare std::copy_n for
    istreambuf_iterator of char types to be friend.
    * testsuite/25_algorithms/copy_n/istreambuf_iterator.cc: New.
    * testsuite/25_algorithms/copy_n/istreambuf_iterator_neg.cc: New.

Tested under Linux x86_64, normal and debug modes.

Ok to commit ?


This is a nice improvement, I just have a few minor comments on it...


François




diff --git a/libstdc++-v3/include/bits/stl_algo.h 
b/libstdc++-v3/include/bits/stl_algo.h
index 478f012def8..ec651e2cc45 100644
--- a/libstdc++-v3/include/bits/stl_algo.h
+++ b/libstdc++-v3/include/bits/stl_algo.h
@@ -771,6 +771,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
 _OutputIterator __result, random_access_iterator_tag)
{ return std::copy(__first, __first + __n, __result); }

+  template
+typename enable_if<__is_char<_CharT>::__value,
+  _OutputIterator>::type


We have __enable_if_t that can be used to simplify this a bit:

   __enable_if_t<__is_char<_CharT>::__value, _OutputIterator>

(The other declarations in bits/streambuf_iterator.h would need to be
changed to use that as well).


+copy_n(istreambuf_iterator<_CharT, char_traits<_CharT> >,


std::copy_n is only declared for C++11 and later, so you don't need
the space between the closing angle brackets: >>


+  _Size __n, _OutputIterator __result);
+
  /**
   *  @brief Copies the range [first,first+n) into [result,result+n).
   *  @ingroup mutating_algorithms
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h 
b/libstdc++-v3/include/bits/streambuf_iterator.h
index 2f4ff494a3a..c682fa91bde 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -80,6 +80,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
__copy_move_a2(istreambuf_iterator<_CharT2>,
   istreambuf_iterator<_CharT2>, _CharT2*);

+#if __cplusplus >= 201103L
+  template
+   friend typename enable_if<__is_char<_CharT2>::__value,
+ _OutputIterator>::type
+   copy_n(istreambuf_iterator<_CharT2>, _Size, _OutputIterator);
+#endif
+
  template
friend typename __gnu_cxx::__enable_if<__is_char<_CharT2>::__value,
istreambuf_iterator<_CharT2> >::__type
@@ -367,6 +374,50 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
  return __result;
}

+#if __cplusplus >= 201103L
+  template
+typename enable_if<__is_char<_CharT>::__value, _OutputIterator>::type
+copy_n(istreambuf_iterator<_CharT> __it, _Size __n,
+  _OutputIterator __result)
+{
+  if (__n == 0)
+   return __result;
+
+  __glibcxx_assert(__n > 0);
+  __glibcxx_requires_cond(!__it._M_at_eof(),
+ _M_message(__gnu_debug::__msg_inc_istreambuf)
+ ._M_iterator(__it));


Is this assertion necessary? Doesn't trying to read from the streambuf
also check the same condition, and so will already fail?


+
+  using traits_type = typename istreambuf_iterator<_CharT>::traits_type;


This is just char_traits<_CharT>, right?


+  const auto __eof = traits_type::eof();
+
+  auto __sb = __it._M_sbuf;
+  while (__n > 0)
+   {
+ streamsize __size = __sb->egptr() - __sb->gptr();
+ if (__size == 0)
+   {
+ if (traits_type::eq_int_type(__sb->underflow(), __eof))
+   {
+ __glibcxx_requires_cond(__n == 0,
+   _M_message(__gnu_debug::__msg_inc_istreambuf)
+ ._M_iterator(__it));
+ break;
+   }
+
+ __size =  __sb->egptr() - __sb->gptr();
+   }
+
+ streamsize __xsize = std::min(__size, __n);
+ __result = std::copy(__sb->gptr(), __sb->gptr() + __xsize, __result);
+ __sb->__safe_gbump(__xsize);
+ __n -= __xsize;
+   }
+
+  return __result;
+} 


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