On Thu, 10 Oct 2019 at 21:57, François Dumont <frs.dum...@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.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 >> >> >