stream iterators into different streams compare equal

• Key: STDCXX-645
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Martin Sebor
• Reporter: Mark Brown
• Component/s: 24. Iterators
• Affects Version/s: 4.1.3, 4.2.0
• Fix Version/s: 4.2.1

Description

As Travis says in his reply to my post here:
http://www.nabble.com/stream-iterators-into-different-streams-compare-equal--tf4721505.html#a13498487:

Given 24.5.1.1 p1 and p2, it is pretty clear to me that the two iterators are both non-end-of-stream type, and they are both created on different streams. The streams are different, so the iterators should not compare equal. I guess one could claim that 24.5.1.2 p6 conflicts with 24.5 p3 because 'end-of-stream' isn't clearly defined, but in this particular case that doesn't matter.

This program aborts with stdcxx but not with gcc:

#include <assert.h>
#include <iterator>
#include <sstream>

int main ()
{
    std::istringstream a ("1");
    std::istream_iterator<int> i (a);

    std::istringstream b ("2");
    std::istream_iterator<int> j (b);

    assert (!(i == j));
}

Comments

[Mark Brown]

The patch is simple:

— /home/mbrown/stdcxx/include/rw/_streamiter.h 2007-03-27 22:32:05.000000000 -0600
+++ /home/mbrown/stdcxx-4.2.0/include/rw/_streamiter.h 2007-11-03 10:08:55.000000000 -0600
@@ -121,7 +121,7 @@
operator== (const istream_iterator<_TypeT, _CharT, _Traits, _Distance>& __x,
const istream_iterator<_TypeT, _CharT, _Traits, _Distance>& __y)

{ - return (__x._C_strm && !!*__x._C_strm) == (__y._C_strm && !!*__y._C_strm); + return __x._C_strm == __y._C_strm; }

[Travis Vitek]

24.5.1 p1 says that an istream_iterator becomes an end-of-stream iterator when the end of stream is reached. So checks are required so that two iterators that become end-of-stream iterators will compare equal.

#include <assert.h>
#include <iterator>
#include <sstream>

int main ()
{
std::istringstream a ("1");
std::istream_iterator<int> i (a);
++i;

std::istringstream b ("2");
std::istream_iterator<int> j (b);
++j;

assert (i == j);

return 0;
}

This method needs to return true if both iterators are end-of-stream iterators, or they have the same non-null stream pointer. Something like this is more appropriate given the implementation of the other methods

template <class _TypeT, class _CharT, class _Traits, class _Distance>
bool
operator== (const istream_iterator<_TypeT, _CharT, _Traits, _Distance>& __x,
const istream_iterator<_TypeT, _CharT, _Traits, _Distance>& __y)
{
if (__x._C_strm != __y._C_strm) { const bool __x_eos = !__x._C_strm || !*__x._C_strm; const bool __y_eos = !__y._C_strm || !*__y._C_strm; // iterators on different streams equal iff they are both eos return __x_eos && __y_eos; }

return true; // iterators on same stream are always equal
}

The other option would be to change our internal definition of end-of-stream to be istream_iterators with a NULL stream pointer. Then operator++ could be changed to invalidate the stream pointer when the stream reaches eos. I think this would work.

istream_iterator& operator++ () {
if (_C_strm && !!*_C_strm)
*_C_strm >> _C_val;
else
_C_strm = 0;
return *this;
}

[Martin Sebor]

Good catch! I thought these checks were already in place but I see they're not.

IMO, the decision where to add them should be made with an eye toward minimizing the performance penalty in the common case. I.e., what's more likely to be called more often: the equality operator or the increment operator on an iterator?

[Travis Vitek]

In most cases the iterator is going to be used in a loop which calls op!= and op++ repeatedly. In this case op== would get called one time more than op+. Of course there are some scenerios where op+ is called more frequently than op== [std::distance or std::advance], but I don't believe that those are the most common use cases for an istream_iterator.

If we fix op+, then op== becomes a pointer comparison and op+ gets an additional assignment. I think fixing op++ would be the preferred solution if we are just counting instructions.

[Martin Sebor]

I would have been tempted to say that operator==() will be called much more often than operator++(). I looked at num_get and money_get (both of which actually use istreambuf_iterator, not istream_iterator), to see if I could find any support for my hypothesis. Here's what I found:

num_get calls both functions exactly once in each iteration of the loop. It also dereferences the iterator exactly once. There is exactly one such call for each of the operators in the body of the loop.

money_get makes up to four calls to operator++() and operator*() in each iteration, and five calls to operator==() (or the inverse of it). There are four and five call sites, respectively, for the functions.

If we can draw any conclusion out of this at all it would seem to be slightly in favor of keeping operator==() light-weight and adding the checking logic to operator++().

[Martin Sebor]

I've been looking into this and I think either there's a real issue in the standard or the text is confusing. My post on the subject to the committee's list is below. The confusing part is that [istream.iterator], p1, duplicates a couple of preconditions for istream_iterator operations (operator*() and operator->()) from Table 96, Input Iterator Requirements, but not others, notably operator+(). This could mean that istream_operator::operator+() is well-defined for end-of-stream iterators or it could be just another piece of redundant text in the standard...

-------- Original Message --------
Subject: can an end-of-stream iterator become a non-end-of-stream one?
Date: Wed, 09 Jan 2008 16:08:52 -0700
From: Martin Sebor <[e-mail]>
Reply-To: [e-mail]
Organization: Rogue Wave Software, Inc.
To: undisclosed-recipients:;

To: C++ libraries mailing list
Message c++std-lib-20003

The istream_iterator description says that "If the end of stream is
reached (operator void*() on the stream returns false), the iterator
becomes equal to the end-of-stream iterator value" without explaining
how exactly this is done.

One possible approach used in practice is for the iterator to set its
in_stream pointer to 0 just like the default ctor does. The problem
with this approach is that the Effects clause for operator++() says
the iterator unconditionally extracts the next value from the stream
by evaluating:

*in_stream >> value;

This can be easily detected by a program setting eofbit or failbit
in exceptions of the associated stream and iterating past the end
of the stream (each past-the-end read should trigger an exception).

Another approach, one that would allow operator++() to attempt to
extract the value even for end-of-stream (EOS) iterators (just as
long as in_stream is non-0) is for the iterator to maintain a flag
indicating whether it has reached the end of the stream. This
approach, however, even though it's also used in practice, isn't
supported by the exposition-only members of the class (no such flag
is shown).

The question then: Is it the intent that a non-EOS that has reached
the EOS become a non-EOS one again after the stream's eofbit flag
has been cleared? I.e., are the assertions in the program below
expected to pass?

sstream strm ("1 ");
istream_iterator eos;
istream_iterator it (strm);
int i;
i = *it++
assert (it == eos);
strm.clear ();
strm << "2 3 ";
assert (it != eos);
i = *++it;
assert (3 == i);

Or is it intended that once an iterator becomes EOS it stays EOS until
the end of its lifetime?

Martin

[Martin Sebor]

Scheduled for 4.2.1, added my initial [under]estimate and enabled code formatting for test case in Description.

[Martin Sebor]

There's no patch here.

Fixed thus: r618883
Regression test added thus: r618878

[Martin Sebor]

Attached proposed fix for the ambiguity in [istream.iterator] submitted to the C++ committee's reflector.

[Martin Sebor]

Turns out that having the iterator check ios::fail() rather than ios::eof() is preferred by other implementers and probably makes more sense from a compatibility standpoint (assuming most implementations actually use ios::operator void*() (or equivalent) to decide when to turn a dereferenceable iterator into an end-of-stream one. The new proposed resolution is what made it into LWG issue 788, available for preview here: http://home.twcny.rr.com/hinnant/cpp_extensions/issues_preview/lwg-active.html#788

Reopening to adjust accordingly.

[Martin Sebor]

Fix and test adjusted according to LWG issue 788 in r619228.

[Martin Sebor]

This time hopefully fixed for good.

[Farid Zaripov]

Merged in 4.2.x branch thus: http://svn.apache.org/viewvc?view=rev&revision=648752

[Martin Sebor]

Regression test committed and verified as passing in nightly builds.
Both fix and test merged to 4.2.x in r648752.