Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ViewableRange seems to be overconstrained #623

Open
CaseyCarter opened this issue Apr 25, 2019 · 6 comments
Open

ViewableRange seems to be overconstrained #623

CaseyCarter opened this issue Apr 25, 2019 · 6 comments

Comments

@CaseyCarter
Copy link
Collaborator

The ViewableRange concept is defined as:

template<class R> concept ViewableRange = 
  Range<R> && (forwarding-range<R> || View<decay_t<R>>);

Note that pointers-to-function and pointers-to-array are never Views, so the decay_t here should be remove_cvref_t. Since A && (B || C) is equivalent to (A && B) || (A && C), this definition is equivalent to:

template<class R> concept ViewableRange = 
  (Range<R> && forwarding-range<R>) || (Range<R> && View<remove_cvref_t<R>>);

The forwarding-range concept is defined:

template<class R> concept forwarding-range = 
  Range<R> && range-impl<R>;

since A && A && B is equivalent to A && B, we can simplify ViewableRange to:

template<class R> concept ViewableRange = 
  forwarding-range<R> || (Range<R> && View<remove_cvref_t<R>>);

Now comes the question: If remove_cvref_t<R> is a View, why should we concern ourselves over whether or not R is a Range? A const specialization of filter_view, for example, is not a Range but View<remove_cvref_t<const filter_view<V, P>>> holds whenever filter_view<V, P> is a valid template-id. Certainly such a const specialization of filter_view "can be converted to a View safely" by making a copy.

Either I'm missing something, or we can reformulate ViewableRange as:

template<class R> concept ViewableRange = 
  View<remove_cvref_t<R>> || forwarding-range<R>;

which would admit more types that "can be converted to a View safely" without admitting any types that cannot be so converted. (Note that I've put the View requirement first here in the expectation that arguments will more commonly be Views than *forwarding-range*s to take advantage of the short-circuiting behavior of constraint-expressions to reduce compile times.)

Proposed Resolution

Wording relative to N4810.

Modify [range.refinements]/4 as follows:

 4 The ViewableRange concept specifies the requirements of a
-Range
 type that can be converted to a View safely.
 
 template<class T>
   concept ViewableRange =
-    Range<T> && (forwarding-range<T> || View<decay_t<T>>);
+    View<remove_cvref_t<T>> || forwarding-range<T>;
@CaseyCarter
Copy link
Collaborator Author

The "requirements of a type" bit is also a bit confusing; we really mean something like "the requirements of an expression E such that decltype((E)) is T which can be converted to a View safely." but eww that's unreadable.

@ericniebler
Copy link
Owner

I'm away from my computer at the moment. Does anything need to change in view::all in light of this?

@CaseyCarter
Copy link
Collaborator Author

CaseyCarter commented Apr 25, 2019

Does anything need to change in view::all in light of this?

No. The proposed change more closely reflects the specification of view::all:

2 The name view::all denotes a range adaptor object. For some subexpression E, the expression view::all(E) is expression-equivalent to:
(2.1) — decay-copy(E) if the decayed type of E models View.
(2.2) — Otherwise, ref_view{E} if that expression is well-formed.
(2.3) — Otherwise, subrange{E}.

When decltype((E)) is T, 2.1 is the View<remove_cvref_t<T>> case, 2.2 is the forwarding-range<T> case when T is an lvalue reference type, and 2.3 is forwarding-range<T> when T is not an lvalue reference type.

CaseyCarter added a commit to CaseyCarter/cmcstl2 that referenced this issue Apr 25, 2019
@ericniebler
Copy link
Owner

If you're replacing decay_t with remove_cvref_t in the definition of ViewableRange, then [range.all]/p2.1 shouldn't be talking about the "decayed type of E".

@CaseyCarter
Copy link
Collaborator Author

remove_cvref_t<T> and "the decayed type of E" are equivalent unless E has function or array type. If it does have function or array type, then "the decayed type of E" is a pointer-to-function or pointer-to-array type that doesn't model View - so the difference is immaterial. "if remove_cvref_t<decltype(E)> models View" would be more precise, but less readable.

CaseyCarter added a commit to CaseyCarter/cmcstl2 that referenced this issue Apr 26, 2019
CaseyCarter added a commit to CaseyCarter/cmcstl2 that referenced this issue Apr 26, 2019
@ericniebler
Copy link
Owner

ericniebler commented Sep 10, 2019

Certainly such a const specialization of filter_view "can be converted to a View safely" by making a copy.

Is this still true once we have move-only views? Had the const filter_view<V, P> been move-only (perhaps P is a move-only lambda), then I think the correct answer is that this type is not a viewable-range. The change you're suggesting here would give the wrong answer.

Fight me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants