ericniebler / range-v3

Range library for C++14/17/20, basis for C++20's std::ranges
Other
4.11k stars 442 forks source link

const any_view is not a Range #824

Open hkalodner opened 6 years ago

hkalodner commented 6 years ago

Hi,

Sorry if I'm doing something silly, but I've run into a problem using any_view. It appears that the problem stems from the fact that Range<any_view<int, ranges::category::random_access | ranges::category::sized>>() is true and Range<const any_view<int, ranges::category::random_access | ranges::category::sized>>() This came up I was trying to stride a sized random access any_view and then store it to an any_view of the same type. stride's size() method is SFINAE'd out based on SizedRange<Rng const> which is false since Rng const is not a range.

Sorry if that explanation was a little confusing, but here's the code that doesn't compile. Does this have something to do with how SizedRange is defined?

auto intsView = ranges::view::ints(10, 20);
using AnyInts = ranges::any_view<int, ranges::category::random_access | ranges::category::sized>;
AnyInts any{intsView};
AnyInts any2{intsView | ranges::view::stride(2)}; // works
AnyInts any3{any | ranges::view::stride(2)}; // compile error

Thanks, Harry

hkalodner commented 6 years ago

I might have posted this a little prematurely. I'm continuing to try to figure out what's going on. It's my understanding that ranges::Range<Rng const>is only true if Rng has const begin and end methods. Therefore, despite the fact that size() is const, SizedRange<Rng const>() is false. This means that CONCEPT_REQUIRES(SizedRange<Rng const>()) is too strict of a requirement here https://github.com/ericniebler/range-v3/blob/c933d27207eaff07586940b37b66658487efd298/include/range/v3/view/stride.hpp#L291 since all we care about is that the range has a const size function, not that it's const iterable. Perhaps this means there needs to be a new concept with a looser restriction.

Creating a Sized concept with the same requirements as SizedRange except without refining Range and using that instead in stride causes the code I posted to compile.

ericniebler commented 5 years ago

I think the original title of this issue, "const any_view is not a Range," was the correct one. There should probably be a way to type-erase a const view and get const-qualified begin() and end() member functions. Something like:

using AnyInts =
    ranges::any_view<
        int,
        ranges::category::random_access |
            ranges::category::sized |
            ranges::category::const_>;

?

hkalodner commented 5 years ago

That would certainly solve my problem as well as be a valuable addition to the library.

In hindsight, I'd probably describe this as two separate issues, the first as you've described, and the second being that SizedRange<Rng const> is over constraining when applied in ranges::view::stride as well as other similar places.

In those situations, ranges::size is being immediately called on the const base view and thus it doesn't matter whether the range has const-qualified begin() and end() members as long as there is a const-qualified size() member.

I resolved my problem through reducing the constraints, but I wasn't certain whether my changes made sense in the broader scope of the library.

hkalodner commented 5 years ago

Also I just wanted to say I've really enjoyed using the library! I've made extensive use of it in my blockchain analysis tool BlockSci which provides an object-oriented python query interface depending on a combination of your library and pybind11.

ericniebler commented 5 years ago

the second being that SizedRange<Rng const> is over constraining when applied in ranges::view::stride as well as other similar places.

I hear what you're saying, but I disagree that it is a problem that needs fixing. Concepts and requirements are "chunky". They sometimes over-constrain, and that is OK. The win is fewer concepts the user needs to understand, and greater implementor freedom.

It also results in concepts that are semantically meaningful. I know that something that models SizedRange also models Range. I don't have to slice the concept up so I can ask "does this compile? does that? what are the semantics?" Either something is a sized range or it is not. Simple.

The semantics of a sized range are that if I call size(r), I get the same result as if I called distance(r.begin(). r.end()), and I get the result in O(1) guaranteed. In order for those semantics to make sense, r needs to be a range with begin() and end().

What you're asking for is a semantically meaningless HasSize concept, and I oppose that.

hkalodner commented 5 years ago

That make's total sense to me. I appreciate you taking the time to explain your philosophy here!

I had gone for the simplest fix possible to get my code working, but clearly your suggested fix fits into the overall framework of the library much better.