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


On Fri, 4 Oct 2019 at 07:01, François Dumont <frs.dum...@gmail.com <mailto:frs.dum...@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

No, I wasn't aware about this issue, I'll have a look.

Thanks

Reply via email to