Re: Make tests less istreambuf_iterator implementation dependent

[Jonathan Wakely <jwakely@redhat.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.