istream_range filtered with take(N) should stop reading at N about range-v3 HOT 14 CLOSED

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.

from range-v3.

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

from range-v3.

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.

from range-v3.

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.

from range-v3.

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.

from range-v3.

@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.

from range-v3.

@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?

from range-v3.

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

from range-v3.

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.

from range-v3.

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

from range-v3.

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.

from range-v3.

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.

from range-v3.

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.

from range-v3.

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.

from range-v3.