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.
