Add std::copy_n overload for istreambuf_iterator
François Dumont
frs.dumont@gmail.com
Thu Oct 10 19:57:00 GMT 2019
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 <frs.dumont@gmail.com
> <mailto: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
>
More information about the Libstdc++
mailing list