From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89740 invoked by alias); 14 Oct 2019 13:06:47 -0000 Mailing-List: contact libstdc++-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libstdc++-owner@gcc.gnu.org Received: (qmail 89720 invoked by uid 89); 14 Oct 2019 13:06:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,HTML_MESSAGE,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=chair, H*r:libstdc, sounds, approximately X-HELO: mail-lf1-f42.google.com Received: from mail-lf1-f42.google.com (HELO mail-lf1-f42.google.com) (209.85.167.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 14 Oct 2019 13:06:43 +0000 Received: by mail-lf1-f42.google.com with SMTP id d17so11772284lfa.7 for ; Mon, 14 Oct 2019 06:06:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Y9bW+Z2nv9YhxtXLgqdC8tff4Ezk2RpWuduGTAvAQBI=; b=lFUew9Zcy5fTm2qnb33gfhqCQI/GApZGw+/5gDuG4/xEFjyyK8+fYo1Smg4K6CSfrD QqiJWe0YJB5HG2K2H8TDmpf9Hp4HSxDi/oaSksUs4Lel3azdit6v7H8BwmmvTiGuSrnA 4OqNnQynz6brhw4pAnjHyG5XSkKs9x7gC3YzVdSCuTLjJypmyHAKQr/IUQSLNHh0sMq7 qg7FWfvSslErXi3uOeOfx4SZiE5CPRgKFWwToRumvS/eVicmamWzbPL9znxXyYuAcYoP 0tcKr0/ROhP8kMMAf03dOPStDusUlLsikab3t+D9VLiS7C8TYIx5Szd7eFVMa3pXrJhk DtmA== MIME-Version: 1.0 References: <20190927110056.GA18624@redhat.com> <4717f824-2fdc-0c12-dd50-82b6d937d485@gmail.com> <22170aff-ace5-b839-21eb-89e005e896d2@gmail.com> In-Reply-To: <22170aff-ace5-b839-21eb-89e005e896d2@gmail.com> From: Christophe Lyon Date: Mon, 14 Oct 2019 13:06:00 -0000 Message-ID: Subject: Re: Add std::copy_n overload for istreambuf_iterator To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: Jonathan Wakely , "libstdc++@gcc.gnu.org" , gcc-patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-SW-Source: 2019-10/txt/msg00051.txt.bz2 On Thu, 10 Oct 2019 at 21:57, Fran=C3=A7ois Dumont w= rote: > 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=C3=A7ois > > On 10/9/19 10:18 PM, Christophe Lyon wrote: > > > > On Fri, 4 Oct 2019 at 07:01, Fran=C3=A7ois Dumont = wrote: > >> On 9/27/19 1:00 PM, Jonathan Wakely wrote: >> > On 19/07/19 23:37 +0200, Fran=C3=A7ois Dumont wrote: >> >> It sounds reasonable to overload std::copy_n for istreambuf_iterator. >> > I wonder whether it's worth doing: >> > >> > #if __cplusplus >=3D 201703L >> > if constexpr (is_same_v<_OutputIterator, _CharT*>) >> > return __result + __it._M_sbuf->sgetn(__result, __n); >> > else >> > { >> > #endif >> > ... >> > #if __cplusplus >=3D 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 >=3D 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 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 fr= ee >> >> +// 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() =3D=3D '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 =3D=3D "12000" ); >> > >> > And equal to ostr.begin() + 2. >> > >> >> + >> >> + std::copy_n(istrb_ite(ss), 3, ostr.begin() + 2); >> >> + VERIFY( ostr =3D=3D "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 =3D=3D "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 declaratio= n. >> (__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 =3D=3D "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=C3=A7ois >> >> >