From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25355 invoked by alias); 3 Oct 2017 14:20:28 -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 24106 invoked by uid 89); 3 Oct 2017 14:20:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=chart, demonstrate, justify X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 03 Oct 2017 14:20:26 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 3CDD8883D1; Tue, 3 Oct 2017 14:20:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3CDD8883D1 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jwakely@redhat.com Received: from localhost (unknown [10.33.36.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id BF05F4DA32; Tue, 3 Oct 2017 14:20:24 +0000 (UTC) Date: Tue, 03 Oct 2017 14:20:00 -0000 From: Jonathan Wakely To: =?iso-8859-1?Q?Fran=E7ois?= Dumont Cc: "libstdc++@gcc.gnu.org" , gcc-patches Subject: Re: Make tests less istreambuf_iterator implementation dependent Message-ID: <20171003142023.GW4582@redhat.com> References: <154b3b11-5b95-2c81-fb4d-408e9e0db374@gmail.com> <20170928121213.GM4582@redhat.com> <20170928215632.GQ4582@redhat.com> <2285784f-dfce-3e48-30bf-902fe1d157b8@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2285784f-dfce-3e48-30bf-902fe1d157b8@gmail.com> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.8.3 (2017-05-23) X-IsSubscribed: yes X-SW-Source: 2017-10/txt/msg00007.txt.bz2 On 02/10/17 07:43 +0200, François Dumont wrote: >On 28/09/2017 23:56, Jonathan Wakely wrote: >>On 28/09/17 21:59 +0200, François Dumont wrote: >>> >>>The current istreambuf_iterator implementation capture the current >>>streambuf state each time it is tested for eof or evaluated. This >>>is why I considered those tests as fragile. >> >>Yes, and I think that's non-conforming. >> > >Good, then we have something to fix. > >>As I said in the other thread, I'd really like to see references to >>the standard used to justify any changes to our istreambuf_iterator > >I think _M_c has been introduced to cover this paragraph of the Standard: > >24.6.3.1 > >"1. Class istreambuf_iterator::proxy is for exposition >only. An implementation is permit- >ted to provide equivalent functionality without providing a class with >this name. Class istreambuf_- >iterator::proxy provides a temporary placeholder as the >return value of the post- >increment operator (operator++). It keeps the character pointed to by >the previous value of the iterator for >some possible future access to get the character." > >This is why it is being set in operator++(int): > >    istreambuf_iterator __old = *this; >    __old._M_c = _M_sbuf->sbumpc(); > >It is also the reason why libstdc++ fails: >http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp > > >It looks like this test is simply not Standard conformant. I think you're right, because it relies on a property that may not be true: any copies of the previous value of r are no longer required either to be dereferenceable or to be in the domain of ==. >I guess at some point _M_c started to be used also to cache the >streambuf::sgetc resulting in current situation. And arguably that is not "equivalent functionality" to returning a proxy object. Although the standard seems a bit underspecified. >In attached patch I limit usage of _M_c to cover 24.6.3.1.1 point and >so made additional changes to 2.cc test case to demonstrate it. Doesn't the modified test PASS anyway, even without changing how _M_c is used? Generally this looks good (I'm not sure if it addresses Petr's concerns, but I don't think his "stop and go" usage is valid anyway - it's certainly not portable or guaranteed by the standard). >@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > : _M_sbuf(0), _M_c(traits_type::eof()) { } > > #if __cplusplus >= 201103L >- istreambuf_iterator(const istreambuf_iterator&) noexcept = default; >+ istreambuf_iterator(const istreambuf_iterator&) = default; Please don't remove this. It's present in the standard, and it ensures we don't accidentally add a member that makes the default noexcept(false). > > ~istreambuf_iterator() = default; > #endif >@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > char_type > operator*() const > { >+ int_type __c = _M_get(); >+ > #ifdef _GLIBCXX_DEBUG_PEDANTIC > // Dereferencing a past-the-end istreambuf_iterator is a > // libstdc++ extension >- __glibcxx_requires_cond(!_M_at_eof(), >+ __glibcxx_requires_cond(!_S_at_eof(__c), > _M_message(__gnu_debug::__msg_deref_istreambuf) > ._M_iterator(*this)); > #endif >- return traits_type::to_char_type(_M_get()); >+ return traits_type::to_char_type(__c); > } > > /// Advance the iterator. Calls streambuf.sbumpc(). > istreambuf_iterator& > operator++() > { >- __glibcxx_requires_cond(!_M_at_eof(), >+ __glibcxx_requires_cond(_M_sbuf && >+ (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())), > _M_message(__gnu_debug::__msg_inc_istreambuf) > ._M_iterator(*this)); >- if (_M_sbuf) >- { This check should be unnecessary, because it's undefined to increment and end-of-stream iterator, but I wonder if anyone is relying on it being currently a no-op, rather than crashing. Let's remove the check, and if it causes too many problems we can restore it as: if (__builtin_expect(_M_sbuf != 0, 1)) >+ > _M_sbuf->sbumpc(); > _M_c = traits_type::eof(); >- } > return *this; > } > >@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > istreambuf_iterator > operator++(int) > { >- __glibcxx_requires_cond(!_M_at_eof(), >+ __glibcxx_requires_cond(_M_sbuf && >+ (!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())), > _M_message(__gnu_debug::__msg_inc_istreambuf) > ._M_iterator(*this)); > > istreambuf_iterator __old = *this; >- if (_M_sbuf) >- { > __old._M_c = _M_sbuf->sbumpc(); > _M_c = traits_type::eof(); >- } > return __old; > } > >@@ -176,26 +175,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > int_type > _M_get() const > { >- const int_type __eof = traits_type::eof(); >- int_type __ret = __eof; >- if (_M_sbuf) >- { >- if (!traits_type::eq_int_type(_M_c, __eof)) >- __ret = _M_c; >- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()), >- __eof)) >- _M_c = __ret; Removing this assignment means that we will call _M_sbuf->sgetc() for every dereference operation, instead of caching the current value in _M_c. I think that's probably OK, because it's unlikely that performance critical code is dereferencing the same iterator repeatedly without incrementing it. And getting rid of the mutable member is a Good Thing. An alternative would be to cache it in operator*() instead of in _M_get(), which would still give the desired change of not updating it when _M_get() is used elsewhere. But would still involve setting a mutable member in a const function. >- else >+ int_type __ret = _M_c; >+ if (_M_sbuf && _S_at_eof(__ret) && _S_at_eof(__ret = _M_sbuf->sgetc())) > _M_sbuf = 0; It's unfortunate that we still have this assignment to a mutable member in a const function, but I think it's necessary to avoid an end-of-stream iterator becoming valid again. >- } > return __ret; > } > > bool > _M_at_eof() const >+ { return _S_at_eof(_M_get()); } >+ >+ static bool >+ _S_at_eof(int_type __c) This should be called _S_is_eof, since it tests if the argument **is** the EOF character. It doesn't test if the stream is **at** EOF, like _M_at_eof does. So with a suitable ChangeLog, the "noexcept" kept on the constructor, and renaming _S_at_eof to _S_is_eof, this is OK for trunk.