istream_range filtered with take(N) should stop reading at N

[blindley]

for (auto i : istream<std::string>(std::cin) | view::take(3))
    std::cout << i << '\n';
for (auto i : istream<std::string>(std::cin) | view::take(3))
    std::cout << i << '\n';

I would expect that snippet to print 6 consecutive strings from standard input. Instead, it prints 3, skips one, and then prints the next 3.

[Rapptz]

Might have to do with this line:

https://github.com/ericniebler/range-v3/blob/master/include/range/v3/istream_range.hpp#L63

It does an extraction in the constructor.

[blindley]

Possible fix. Add a bool to istream_range to determine if the stream is up to date with the current position.
https://gist.github.com/blindley/b2e2f9cf18ceade29a77

[ericniebler]

This might work, but it makes me uncomfortable since a mutating operation is happening in cursor::current and cursor::done, which are const member functions. The mitigating factor is that istream_range only has a non-const begin() member, so mutation during iteration is implicitly allowed. This is just a strange place for the mutation to happen.

The interaction with the view::take adaptor happens because the count and the iterator are both incremented before the new count gets compared to N. We might be able to do something sneaky there, where the underlying iterator isn't incremented if it doesn't need to be. That feels wrong, too; it's likely to screw up something else. Maybe this "fix" is better, or maybe the answer is: Don't Do That. I need to think about it.

[gnzlbg]

IMO the root of this issue is the unexpected interaction of using a view (which shouldn't mutate anything), and the continuously mutating input range.

Is there a situation where not incrementing the iterator in take, if it doesn't need to be incremented, leads to unexpected behavior? I've been trying to construct such a case for non-input ranges but the change doesn't seem to break anything (yet).

One shouldn't need to have a "deeper" understanding of take's internals, and, on a first read of the code snippet above, I expected the input iterator to be advanced 6 times instead of 8. Views are also too tempting not to use them, so...

Maybe we should also check other views like take_while or unique. A mutating input range that can be constructed from an initializer_list might be very useful for testing these things.

[ericniebler]

Is there a situation where not incrementing the iterator in take, if it doesn't need to be incremented, leads to unexpected behavior?

I'm pretty sure that's the wrong fix. I haven't found a case that breaks yet, but it feels all wrong.

I expected the input iterator to be advanced 6 times instead of 8.

I think it's advanced 6 times. That's not the problem.

[ericniebler]

@blindley, regarding https://gist.github.com/blindley/b2e2f9cf18ceade29a77, I think you'll find that your suggested fix still has problems. Tell me what this code does (taking 0 elements instead of 3):

for (auto i : istream<std::string>(std::cin) | view::take(0))
    std::cout << i << '\n';
for (auto i : istream<std::string>(std::cin) | view::take(0))
    std::cout << i << '\n';

I would expect this code to leave std::cin alone, but I'm pretty sure it won't.

I just don't think there's any good answer here.

[blindley]

@ericniebler
I tested it with view::take(0) and it seems to leave cin alone, exactly as I would expect.

Did you have different results? Or is there something obvious I am missing in my test?

[blindley]

Come to think of it, that previous test wasn't very convincing. Perhaps this will be.

[ericniebler]

OK, try this:

#include <sstream>
#include <range/v3/core.hpp>
#include <range/v3/istream_range.hpp>

int main()
{
    std::stringstream str{"1 2 3 4 5 6 7 8 9"};
    if(ranges::empty(ranges::istream<int>(str)))
        std::cout << "empty!\n";
    if(ranges::empty(ranges::istream<int>(str)))
        std::cout << "empty!\n";
    if(ranges::empty(ranges::istream<int>(str)))
        std::cout << "empty!\n";
    int i;
    str >> i;
    std::cout << "first int : " << i << "\n";
}

For me this prints:

first int : 4

Merely asking the istream range if it's empty is reading an int and throwing it away.

[blindley]

Okay, I concur. I'll see if that can be fixed.

[blindley]

I've just noticed that the current version of istream_range seems to suffer from the same problem with empty() (not that I'm trying to excuse the bug in my own version).

As of now, the only potential solution that is coming to mind is really nasty. It would involve storing a string buffer in the range to serve as a peek into the std::istream. Things would get really nasty for user-defined types where we have no way of knowing how much data their operator>> overload might consume. I will continue thinking on this though.

[ericniebler]

Right now, I think the best solution is to provide access to the cached value, so that if you stop reading values before the stream runs out, you have a way to get the item that would be lost. That's the best I've come up with.

[blindley]

Yeah, that's probably much better than what I was thinking. Although I'm curious how it would work (as far as the interface goes), and if you mean to do that instead of, or in addition to something that would solve the problem presented in my original post.

[ericniebler]

I don't think there is a solution to your problem. I think istream_range should have a member function to retrieve the cached element. And I think so the adaptors need a base() member function to fetch the underlying adapted range. That's the best we can do, IMO.