From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 49912 invoked by alias); 3 Oct 2017 20:39:35 -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 46997 invoked by uid 89); 3 Oct 2017 20:39:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-22.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_INFOUSMEBIZ,KAM_LAZY_DOMAIN_SECURITY,RDNS_DYNAMIC autolearn=ham version=3.3.2 spammy=VERIFY, streams, H*M:info, life X-Spam-User: qpsmtpd, 2 recipients X-HELO: void-ptr.info Received: from pppoe.185.44.68.223.lanport.ru (HELO void-ptr.info) (185.44.68.223) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Oct 2017 20:39:31 +0000 Received: from ptr by void-ptr.info with local (Exim 4.72) (envelope-from ) id 1dzTyS-0006rP-2X; Tue, 03 Oct 2017 23:39:28 +0300 Date: Tue, 03 Oct 2017 20:39:00 -0000 From: Petr Ovtchenkov To: Jonathan Wakely Cc: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [PATCH] libstdc++: istreambuf_iterator keep attached streambuf Message-ID: <20171003233927.6b5a151c@void-ptr.info> In-Reply-To: <20170928123806.GN4582@redhat.com> References: <20170928103425.GG4582@redhat.com> <20170928150643.2f667ec9@void-ptr.info> <20170928123806.GN4582@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00009.txt.bz2 On Thu, 28 Sep 2017 13:38:06 +0100 Jonathan Wakely wrote: > On 28/09/17 15:06 +0300, Petr Ovtchenkov wrote: > >On Thu, 28 Sep 2017 11:34:25 +0100 > >Jonathan Wakely wrote: > > > >> On 23/09/17 09:54 +0300, Petr Ovtchenkov wrote: > >> >istreambuf_iterator should not forget about attached > >> >streambuf when it reach EOF. > >> > > >> >Checks in debug mode has no infuence more on character > >> >extraction in istreambuf_iterator increment operators. > >> >In this aspect behaviour in debug and non-debug mode > >> >is similar now. > >> > > >> >Test for detached srteambuf in istreambuf_iterator: > >> >When istreambuf_iterator reach EOF of istream, it should not > >> >forget about attached streambuf. > >> From fact "EOF in stream reached" not follow that > >> >stream reach end of life and input operation impossible > >> >more. > >> >--- > >> > libstdc++-v3/include/bits/streambuf_iterator.h | 41 +++++++-----= --- > >> > .../24_iterators/istreambuf_iterator/3.cc | 61 ++++++++++++= ++++++++++ > >> > 2 files changed, 80 insertions(+), 22 deletions(-) > >> > create mode 100644 libstdc++-v3/testsuite/24_iterators/istreambuf_it= erator/3.cc > >> > > >> >diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h > >> >b/libstdc++-v3/include/bits/streambuf_iterator.h index f0451b1..45c3d= 89 100644 > >> >--- a/libstdc++-v3/include/bits/streambuf_iterator.h > >> >+++ b/libstdc++-v3/include/bits/streambuf_iterator.h > >> >@@ -136,12 +136,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> > istreambuf_iterator& > >> > operator++() > >> > { > >> >- __glibcxx_requires_cond(!_M_at_eof(), > >> >+ __glibcxx_requires_cond(_M_sbuf, > >> > _M_message(__gnu_debug::__msg_inc_istreambuf) > >> > ._M_iterator(*this)); > >> > if (_M_sbuf) > >> > { > >> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC > >> >+ int_type _tmp =3D > >> >+#endif > >> > _M_sbuf->sbumpc(); > >> >+#ifdef _GLIBCXX_DEBUG_PEDANTIC > >> >+ __glibcxx_requires_cond(!traits_type::eq_int_type(_tmp,traits_t= ype::eof()), > >> >+ _M_message(__gnu_debug::__msg_inc_istreambuf) > >> >+ ._M_iterator(*this)); > >> >+#endif > >> > _M_c =3D traits_type::eof(); > >> > } > >> > return *this; > >> >@@ -151,14 +159,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> > istreambuf_iterator > >> > operator++(int) > >> > { > >> >- __glibcxx_requires_cond(!_M_at_eof(), > >> >+ _M_get(); > >> >+ __glibcxx_requires_cond(_M_sbuf > >> >+ && !traits_type::eq_int_type(_M_c,traits_type::eof()), > >> > _M_message(__gnu_debug::__msg_inc_istreambuf) > >> > ._M_iterator(*this)); > >> > > >> > istreambuf_iterator __old =3D *this; > >> > if (_M_sbuf) > >> > { > >> >- __old._M_c =3D _M_sbuf->sbumpc(); > >> >+ _M_sbuf->sbumpc(); > >> > _M_c =3D traits_type::eof(); > >> > } > >> > return __old; > >> >@@ -177,18 +187,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> > _M_get() const > >> > { > >> > const int_type __eof =3D traits_type::eof(); > >> >- int_type __ret =3D __eof; > >> >- if (_M_sbuf) > >> >- { > >> >- if (!traits_type::eq_int_type(_M_c, __eof)) > >> >- __ret =3D _M_c; > >> >- else if (!traits_type::eq_int_type((__ret =3D _M_sbuf->sgetc()), > >> >- __eof)) > >> >- _M_c =3D __ret; > >> >- else > >> >- _M_sbuf =3D 0; > >> >- } > >> >- return __ret; > >> >+ if (_M_sbuf && traits_type::eq_int_type(_M_c, __eof)) > >> >+ _M_c =3D _M_sbuf->sgetc(); > >> >+ return _M_c; > >> > } > >> > > >> > bool > >> >@@ -339,7 +340,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> > typedef typename __is_iterator_type::streambuf_type streambuf= _type; > >> > typedef typename traits_type::int_type int_type; > >> > > >> >- if (__first._M_sbuf && !__last._M_sbuf) > >> >+ if (__first._M_sbuf && (__last =3D=3D istreambuf_iterator<_Cha= rT>())) > >> > { > >> > streambuf_type* __sb =3D __first._M_sbuf; > >> > int_type __c =3D __sb->sgetc(); > >> >@@ -374,7 +375,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> > typedef typename __is_iterator_type::streambuf_type streambuf= _type; > >> > typedef typename traits_type::int_type int_type; > >> > > >> >- if (__first._M_sbuf && !__last._M_sbuf) > >> >+ if (__first._M_sbuf && (__last =3D=3D istreambuf_iterator<_Cha= rT>())) > >> > { > >> > const int_type __ival =3D traits_type::to_int_type(__val); > >> > streambuf_type* __sb =3D __first._M_sbuf; > >> >@@ -395,11 +396,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >> > else > >> > __c =3D __sb->snextc(); > >> > } > >> >- > >> >- if (!traits_type::eq_int_type(__c, traits_type::eof())) > >> >- __first._M_c =3D __c; > >> >- else > >> >- __first._M_sbuf =3D 0; > >> >+ __first._M_c =3D __c; > >> > } > >> > return __first; > >> > } > >> >diff --git a/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/= 3.cc > >> >b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc new fi= le mode 100644 > >> >index 0000000..803ede4 > >> >--- /dev/null > >> >+++ b/libstdc++-v3/testsuite/24_iterators/istreambuf_iterator/3.cc > >> >@@ -0,0 +1,61 @@ > >> >+// { dg-options "-std=3Dgnu++17" } > >> >+ > >> >+// Copyright (C) 2017 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 > >> >+// . > >> >+ > >> >+#include > >> >+#include > >> >+#include > >> >+#include > >> >+#include > >> >+ > >> >+void test03() > >> >+{ > >> >+ using namespace std; > >> >+ bool test __attribute__((unused)) =3D true; >=20 > This variable should be removed, we don't need these variables any > more. Not a problem, if we will have consensus in principle. >=20 > >> >+ std::stringstream s; > >> >+ char b[] =3D "c2ee3d09-43b3-466d-b490-db35999a22cf"; > >> >+ char r[] =3D "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; > >> >+ char q[] =3D "3c4852d6-d47b-4f46-b05e-b5edc1aa440e"; > >> >+ // 012345678901234567890123456789012345 > >> >+ // 0 1 2 3 > >> >+ s << b; > >> >+ VERIFY( !s.fail() ); > >> >+ > >> >+ istreambuf_iterator i(s); > >> >+ copy_n(i, 36, r); > >> >+ ++i; // EOF reached > >> >+ VERIFY(i =3D=3D std::istreambuf_iterator()); > >> >+ > >> >+ VERIFY(memcmp(b, r, 36) =3D=3D 0); > >> >+ > >> >+ s << q; > >> >+ VERIFY(!s.fail()); > >> >+ > >> >+ copy_n(i, 36, r); > >> > >> This is undefined behaviour. The end-of-stream iterator value cannot > >> be dereferenced. > > > >Within this test istreambuf_iterator in eof state never dereferenced. >=20 > That is quite implementation dependent. >=20 > The libc++ and VC++ implementations fail this test, because once an > istreambuf_iterator has been detected to reach end-of-stream it > doesn't get "reset" by changes to the streambuf. If we will keep even "unspecified" behaviour same, then bug fix/drawback removing become extremely hard: it should be identified as drawback in all libs almost simultaneously. >=20 > The libc++ implementation crashes, because operator=3D=3D on an > end-of-stream iterator sets its streambuf* to null, and any further > increment or dereference will segfault. >=20 > So this is testing something that other implementations don't support, > and isn't justifiable from the standard. I will use N4687 as reference. 27.2.3 par.2 Table 95: ++r Requires: r is dereferenceable. Postconditions: r is dereferenceable or r is past-the-end; any copies of the previous value of r are no longer required either to be dereferenceable or to be in the domain of =3D=3D. (void)r++ equivalent to (void)++r *r++ { T tmp =3D *r; ++r; return tmp; } [BTW, you see that r++ without dereference has no sense, and even more, copies of the previous value of r are no longer required either to be dereferenceable or to be in the domain of =3D=3D. =46rom this follow, that postfix increment operator shouldn't return istreambuf_iterator. ] >=20 > >The test itself simulate "stop and go" istream usage. > >stringstream is convenient for behaviuor illustration, but in "real life" > >I can assume socket or tty on this place. >=20 > At the very minimum we should have a comment in the test explaining > how it relies on non-standard, non-portable behaviour. >=20 > But I'd prefer to avoid introducing more divergence from other > implementations. Standard itself say nothting about "stop and go" scenario. At least I don't see any words pro or contra. But current implementation block usage of istreambuf_iterator with underlying streams like socket or tty, so istreambuf_iterator become almost useless structure for practice. >=20 > >debugmode-dependent behaviour is also issue in this patch; > >I don't suggest it as a separate patch because solutions are intersect. We have three issues with istreambuf_iterator: - debug-dependent behaviour - EOL of istreambuf_iterator when it reach EOF (but this not mean EOL of associated streambuf) - postfix increment operator return istreambuf_iterator, but here expected restricted type, that accept only dereference, if it possible.