Re: Make tests less istreambuf_iterator implementation dependent

[Jonathan Wakely <jwakely at redhat dot com>]

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<charT,traits>::proxy is for exposition 
> only. An implementation is permit-
> ted to provide equivalent functionality without providing a class with 
> this name. Class istreambuf_-
> iterator<charT, traits>::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.