On 03/10/2017 16:20, Jonathan Wakely wrote: > 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? No it doesn't because the first check for eof capture the 'a' char which is never reseted so after string construction it is still pointing to this char and is not an eof iterator. Without the _M_c assignment the iterator is still "live" and so keeps on reflecting the streambuf state. I'll show you some other side effects of this _M_c later. > > 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). No it doesn't address Petr's concern who is more focus on the _M_sbuf nullification part. > >> @@ -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). Ok > >> )), >>                 _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)) > >> -        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. Yes, an alternative could be to reset it in increment operators but it had other side effects. I'll try to send you the result of this approach. > >> -      } >>     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 Attached patch committed with: 2017-10-04  Petr Ovtchenkov          François Dumont      * include/bits/streambuf_iterator.h     (istreambuf_iterator<>::operator*()): Do not capture iterator state     in Debug assertion.     (istreambuf_iterator<>::operator++()): Likewise and remove _M_sbuf check.     (istreambuf_iterator<>::operator++(int)): Likewise.     (istreambuf_iterator<>::_M_get()): Remove _M_c assignment.     (istreambuf_iterator<>::_S_is_eof()): New.     (istreambuf_iterator<>::_M_at_eof()): Adapt, use latter.     (find(istreambuf_iterator<>, istreambuf_iterator<>, _CharT)):     Return an iterator with _M_c set to eof to capture streambuf state     on evaluation.     (testsuite/24_iterators/istreambuf_iterator/2.cc): Add checks. François