[Bug libstdc++/46906] New: istreambuf_iterator is late?

[Bug libstdc++/46906] New: istreambuf_iterator is late?

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

           Summary: istreambuf_iterator is late?
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: marc.glisse@normalesup.org


The following program prints:
11
12
32

With some other libraries I get 11 22 33 (but libstdc++ is not the only one
that outputs 11 12 32).

As far as I understand the standard, operator++ and operator* should just call
sbumpc and sgetc on the underlying streambuf, which happens to be the same for
the two iterators. It looks like the iterator caches the result somehow. Am I
missing some provision for this in the standard?


#include <ios>
#include <istream>
#include <sstream>
#include <iostream>

using namespace std;
int main(){
    istringstream s("1234");
    istreambuf_iterator<char> i1(s);
    istreambuf_iterator<char> i2(i1);
    std::cerr << *i1 << *i2 << '\n';
    ++i2;
    std::cerr << *i1 << *i2 << '\n';
    ++i1;
    std::cerr << *i1 << *i2 << '\n';
}

[Bug libstdc++/46906] istreambuf_iterator is late?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #1 from Paolo Carlini <paolo.carlini at oracle dot com> 2010-12-12 17:38:10 UTC ---
Yes, this is an old issue, as you noticed already other implementations also
decided that performance issue may take precedence here. After so many years
with this scheme, I honestly don't think we want to change this before breaking
the ABI.

[Bug libstdc++/46906] istreambuf_iterator is late?

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #2 from Marc Glisse <marc.glisse at normalesup dot org> 2010-12-12 18:01:41 UTC ---
(In reply to comment #1)
> Yes, this is an old issue,

Oups, sorry, I am not very good at finding old issues in bugzilla...

> as you noticed already other implementations also
> decided that performance issue may take precedence here. After so many years
> with this scheme, I honestly don't think we want to change this before breaking
> the ABI.

Ok, part of my bugreport was to get confirmation that it was indeed a bug
(streams can be confusing at times). I understand from your comment that you
intend to fix it when the ABI is broken for C++0x, which is good enough for me.

Thanks.

[Bug libstdc++/46906] istreambuf_iterator is late?

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #3 from Paolo Carlini <paolo.carlini at oracle dot com> 2010-12-12 18:28:02 UTC ---
To be clear: I remember discussing this issue with Nathan many years ago, when
we noticed that variance among implementations, and that we considered more
strictly conforming not buffering anything (but also much worse perfoming in
many normal circumstances). But now I don't have a final word on that, should
at least collect my old notes. As I mentioned already, anyway, even if we
wanted to do the change now, it would risk breaking compatibility or leaving
the _M_c member unused or other ugly options. Definitely something to be
re-evaluated when the ABI is broken.

[Bug libstdc++/46906] istreambuf_iterator is late?

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

Daniel Krügler <daniel.kruegler at googlemail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |daniel.kruegler at
                   |                            |googlemail dot com

--- Comment #4 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2011-09-05 09:48:17 UTC ---
IMO the example program is broken and cannot be used to proof violation of
contract of the library. This is so, because istreambuf_iterator is an input
iterator and any usage of operator++ potentially invalidates copies of the
iterator (see FDIS, Table 107, expression r++).

[Bug libstdc++/46906] istreambuf_iterator is late?

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #5 from Marc Glisse <marc.glisse at normalesup dot org> 2011-09-05 09:58:19 UTC ---
(In reply to comment #4)
> IMO the example program is broken and cannot be used to proof violation of
> contract of the library. This is so, because istreambuf_iterator is an input
> iterator and any usage of operator++ potentially invalidates copies of the
> iterator (see FDIS, Table 107, expression r++).

You are completely right, thanks. On the other hand, it looks like I can
construct i2 from s (instead of copying from i1) and still hit the same issue
with a valid program. Do you agree? (Hmm, could the standard make it undefined
to interlace uses of an istreambuf_iterator and other operations on the
istreambuf, to allow this behavior?)

[Bug libstdc++/46906] istreambuf_iterator is late?

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #6 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2011-09-05 11:11:26 UTC ---
(In reply to comment #5)
> On the other hand, it looks like I can
> construct i2 from s (instead of copying from i1) and still hit the same issue
> with a valid program. Do you agree? (Hmm, could the standard make it undefined
> to interlace uses of an istreambuf_iterator and other operations on the
> istreambuf, to allow this behavior?)

You still hit the same result, but the result has nothing to do with some
special implementation details of std::istreambuf_iterator:

1) The fact that repeated calls of operator* without intervening operator++
calls produce the same result for a given iterator object is required by
expression *a:

"The expression (void)*a, *a is equivalent to *a."

This explains the repeated values '1' and '1' from it1 and '2' and '2' from
it2.

2) The observation that the last output produces a "3" for the first iterator
is to be expected from the fact, that std::istreambuf_iterator is a shallow
wrapper for the actual stream buffer, as described in [istreambuf.iterator] p1
says: 

"The class template istreambuf_iterator defines an input iterator (24.2.3) that
reads successive characters from the streambuf for which it was constructed." 

Any "external" accesses to that stream buffer (and the usage of an effective
sbumpc() call via the second std::istreambuf_iterator object on the same stream
buffer) obviously changes the state of the stream buffer in the expected way.
The same result would be observed when you would replace the second iterator
object by explicit calls of the stream buffer as follows:

#include <ios>
#include <istream>
#include <sstream>
#include <iostream>

using namespace std;
int main(){
    istringstream s("1234");
    istreambuf_iterator<char> i1(s);
    std::basic_streambuf<char>& b = *s.rdbuf();
    std::cerr << *i1 << (char) b.sgetc() << '\n';
    b.sbumpc();
    char c = b.sgetc();
    std::cerr << *i1 << c << '\n';
    ++i1;
    std::cerr << *i1 << c << '\n';
}

I'm getting:

11
12
32

which is non-distinguishable from the example program with two different
std::istreambuf_iterator objects. I don't see why there would be undefined
behaviour involved.

[Bug libstdc++/46906] istreambuf_iterator is late?

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #7 from Marc Glisse <marc.glisse at normalesup dot org> 2011-09-05 12:38:13 UTC ---
(In reply to comment #6)
> 1) The fact that repeated calls of operator* without intervening operator++
> calls produce the same result for a given iterator object is required by
> expression *a:
> 
> "The expression (void)*a, *a is equivalent to *a."

There are operations between the 2 calls to operator*.

> This explains the repeated values '1' and '1' from it1 and '2' and '2' from
> it2.

Oh, are you saying that this rule has priority over the one that says that
operator* just forwards to sgetc? That would actually require
istreambuf_iterator to keep the last value in cache the way libstdc++ does. If
that's the case, I believe it would be worth mentioning in the standard (it's
not just me, when I filed the bug report I had found at least one
implementation that did not cache, Roguewave (haven't checked more recent
Apache versions)).

For a vector, touching the vector between 2 dereferencing of the same iterator
invalidates the iterator. Here, my guess was that it didn't invalidate the
iterator and we should blindly call sgetc. But your explanation makes sense.

[Bug libstdc++/46906] istreambuf_iterator is late?

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #8 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2011-09-05 12:56:44 UTC ---
(In reply to comment #7)
> Oh, are you saying that this rule has priority over the one that says that
> operator* just forwards to sgetc? 

This was not my intention, but I recognize that my last response can be read
that way.

> That would actually require istreambuf_iterator to keep the last value in cache the way libstdc++ does. 

This is not required, but I don't see why this should be excluded by the
specification. Why do you think that either implementation form could be
considered as non-conforming?

[Bug libstdc++/46906] istreambuf_iterator is late?

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #9 from Marc Glisse <marc.glisse at normalesup dot org> 2011-09-05 14:01:08 UTC ---
(In reply to comment #8)
> Why do you think that either implementation form could be
> considered as non-conforming?

When I read that operator* returns sgetc(), I understand that as 
assert(*i==buf.sgetc()).

If there really is a provision that lets *i return what buf.sgetc() used to
return (I am not convinced the (void)*a,*a thing is it), it would be nice to
remind it in the definition of operator*. And I guess I don't really like this
kind of unspecified behavior... (it is very different from copy elision for
instance)

But it wouldn't be the first time that it is my understanding of the standard
that is at fault ;-)

[Bug libstdc++/46906] istreambuf_iterator is late?

[daniel.kruegler at googlemail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #10 from Daniel Krügler <daniel.kruegler at googlemail dot com> 2011-09-05 14:17:14 UTC ---
(In reply to comment #9)
> (In reply to comment #8)
> > Why do you think that either implementation form could be
> > considered as non-conforming?
> 
> When I read that operator* returns sgetc(), I understand that as 
> assert(*i==buf.sgetc()).

But as explained below this requires that no further external source modifies
the stream (buffer) at the same time. It is unspecified whether an
implementation caches the read value or not. Both implementations seem to be
conforming, because an input iterator is not required to return an identity
from operator*.

> If there really is a provision that lets *i return what buf.sgetc() used to
> return (I am not convinced the (void)*a,*a thing is it), it would be nice to
> remind it in the definition of operator*.

The "(void)*a, *a" requirements ensures that you can invoke operator* several
times without a value change of the result. This holds for both implementations
if no external changes happen to the stream buffer. I agree that the wording
does not say this very clear.

> And I guess I don't really like this
> kind of unspecified behavior... (it is very different from copy elision for
> instance)
> 
> But it wouldn't be the first time that it is my understanding of the standard
> that is at fault ;-)

You could request an LWG issue for this to clarify the intent or to enforce
only a single valid implementation, provided there are some good reasons for
this.

[Bug libstdc++/46906] istreambuf_iterator is late?

[marc.glisse at normalesup dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #11 from Marc Glisse <marc.glisse at normalesup dot org> 2011-09-05 15:05:56 UTC ---
(In reply to comment #10)
> > When I read that operator* returns sgetc(), I understand that as 
> > assert(*i==buf.sgetc()).
> But as explained below this requires that no further external source modifies
> the stream (buffer) at the same time. It is unspecified whether an
> implementation caches the read value or not.

I am sorry but I still can't see what allows the implementation to cache the
value in the presence of external changes to the streambuf.

> The "(void)*a, *a" requirements ensures that you can invoke operator* several
> times without a value change of the result. This holds for both implementations
> if no external changes happen to the stream buffer.

But here there is an external change, so it doesn't apply.

> You could request an LWG issue for this to clarify the intent

Nah, since it looks clear to you that both behaviors are valid, unless someone
else complains, I'll leave it alone (and I guess I'll close the bug as
invalid).

[Bug libstdc++/46906] istreambuf_iterator is late?

[glisse at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

Marc Glisse <glisse at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |ABI
                 CC|                            |glisse at gcc dot gnu.org

--- Comment #12 from Marc Glisse <glisse at gcc dot gnu.org> 2012-08-22 20:00:11 UTC ---
Adding keyword ABI following Paolo's comment.
(not a bug, according to Daniel, but that's orthogonal to being ABI-related)

[Bug libstdc++/46906] istreambuf_iterator is late?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46906

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This seems related to https://cplusplus.github.io/LWG/issue2366 and the changes
I'm proposing there.