ericniebler / stl2

LaTeX and Markdown source for the Ranges TS/STL2 and associated proposals
88 stars 8 forks source link

`const subrange<I,S,[un]sized>` is not a forwarding-range #592

Open ericniebler opened 5 years ago

ericniebler commented 5 years ago

Given the current wording, the following will not compile:

int *p = ranges::cbegin(ranges::subrange<int*>{});

That's because subrange defines its rvalue begin overload as:

friend constexpr I begin(subrange&& r) { return r.begin(); }

This also has the problem that any type that happens to have subrange as an associated namespace (like split_view<subrange<...>> will find this overload and attempt a conversion from split_view to subrange&&, which could possibly cause weirdness.

Better to define the overload as:

template<class A, class B> concept same-ish = Same<A const, B const>;

template<class I, ...>
struct subrange {
...
  friend constexpr I begin(same-ish<subrange> auto && r) { return r.begin(); }
...
};

It also fixes the problem of a type deriving from subrange and appearing to model forwarding-range when in fact it might not.

We also want to give iota_view a similar treatment (see #575). ref_view's begin/end should take by Same<ref_view> auto r.

Finally, the poison-pill overloads for std::initializer_list are wrong. Should be:

-template<class T> void begin(initializer_list<T> &&) = delete;
+template<class T> void begin(initializer_list<T>) = delete;

Likewise for end.

Proposed Resolution

In [range.access.begin]/p1.3, change the poison-pill overloads as follows:

 template<class T> void begin(T&&) = delete;
-template<class T> void begin(initializer_list<T>&&) = delete;
+template<class T> void begin(initializer_list<T>) = delete;

To the synopsis in [range.subrange]/p1, add:

template<class A, class B>
concept same-ish = // exposition only
  same_as<A const, B const>;

In the class synopsis of subrange (same section), change the begin/end friend functions to be as follows:

friend constexpr I begin(same-ish<subrange> auto && r) { return r.begin(); }
friend constexpr S end(same-ish<subrange> auto && r) { return r.end(); }

In the synopsis of ref_view in [range.ref.view]/p1, change the begin/end friend functions as follows (editors note: the use of same_as here instead of same-ish is intentional):

-friend constexpr iterator_t<R> begin(ref_view r)
+friend constexpr iterator_t<R> begin(same_as<ref_view> auto r)
 { return r.begin(); }
-friend constexpr sentinel_t<R> end(ref_view r)
+friend constexpr sentinel_t<R> end(same_as<ref_view> auto r)
 { return r.end(); }

To the class synopsis of iota_view in [range.iota.view], add the following begin/end friend functions:

friend constexpr W begin(same-ish<iota_view> auto && r) { return r.begin(); }
friend constexpr auto end(same-ish<iota_view> auto && r) { return r.end(); }
brevzin commented 5 years ago

Does this formulation work, @ericniebler?

friend constexpr I begin(same_as<subrange> auto r) { return r.begin(); }

Or even:

friend constexpr I begin(subrange r) { return r.begin(); }

It also fixes the problem of a type deriving from subrange and appearing to model forwarding-range when in fact it might not.

This one I don't think is a problem, since the poison-pill would be a better match for the derived types: https://godbolt.org/z/JK4KQ4

CaseyCarter commented 5 years ago

For the record: I vastly prefer the simple "take the forwarding-range by value" formulation to the same_as<foo> auto hack.

brevzin commented 5 years ago

Yeah I started writing that comment before I realized that the by-value version actually fixes the problem that Eric was concerned about. I'm not sure there is any advantage to the template version?

ericniebler commented 5 years ago

If the begin function in question is a function and not a function template, then whenever it is found by ADL, the compiler will consider conversation sequences to its argument. That's what I was saying above about potential weirdness.

UPDATE: Or does the poison pill fix that also? I guess it will throw all the overloads in a big pot and try them all, and only fail if the poison pill is selected. "Trying them all" might involve looking for a conversion sequence to subrange and that has the potential to introduce hard errors or constraint recursion. Call me paranoid.

brevzin commented 5 years ago

Yeah the poison pill fixes that too. Non-template version, both derived and convertible: https://godbolt.org/z/QUhHdI

If this is so concerning, we should just introduce an explicit-opt-in variable template instead of doing overload resolution for an explicit-opt-in non-member. Wouldn't that compile faster anyway?

tcanens commented 5 years ago

The issue is not picking that overload. It's the process of determining whether that overload is viable, even if we'll never end up picking it anyway. That can necessitate template instantiations and headaches, and is definitely permitted (if not required).

Example.

ericniebler commented 5 years ago

Thanks, @tcanens. This is the concern, yes. It's not an issue for function templates. So another potential fix would be to define the subrange begin function as:

template<class It, class Sent, subrange_kind Kind>
constexpr It begin( subrange<It, Sent, Kind> rng )
{
    return rng.begin();
}

... and put it either at namespace scope or make it a friend in a private (non-dependent) base of subrange.

EDIT: I suppose we could document it as a member of namespace std::ranges alongside subrange. I think an implementation that makes it a friend in a private base would be conforming. Users can't (?) tell the difference in a conforming program since they are not allowed to take the address of functions in std::.

EDIT 2: Making it a function template in namespace std::ranges puts it in the same declarative region as the ranges::begin CPO, which is obviously ill-formed. But we already have this "problem": as a friend function in subrange it collides with ranges::begin. We have been waving our hands about that, trusting implementors to solve the problem in the obvious[*] way: by putting it in a different declarative region where ADL will still find it.

[*]: Nothing about this is obvious.

brevzin commented 5 years ago

I would argue that if:

friend constexpr I begin(subrange r) { return r.begin(); }

isn't considered good enough, then we should rethink non-member begin as the customization point and seriously consider just a variable template type trait. This is already super complicated!

My concerns about accidental opt-in to forwarding-range have been alleviated, but I'm not sure what the relative benefit is of the non-member+poison-pill design vs just a type trait? The current approach seems more compile-expensive?

ericniebler commented 5 years ago

Fair.

Though I should say that very few people will ever be troubled to opt-in to this, and that if they do, they will probably do so with a namespace-scoped begin function or function template, and that even if they write a non-template friend thing, they're still extremely unlikely to run into problems due to bad conversion sequences. My reason for using begin this way was to avoid creating Yet Another Trait, and I don't love traits.

what the relative benefit is of the non-member+poison-pill design vs just a type trait?

We have the begin CPO and the poison-pill anyway. Why add a trait when we can just use what we have? This is my rationale. I can see the other side of this also.

ericniebler commented 5 years ago

In my NB comment, I suggested:

template <... >
struct subrange {
  friend constexpr I begin(same_as<subrange> auto r) { return r.begin(); }
  friend constexpr S end(same_as<subrange> auto r) { return r.end(); }
};

I think this is actually a mistake, particularly for end. If r is an xvalue, and if I is a move-only iterator, then simply calling end will destroy the begin position.

I think I had it right the first time with the same-ish<subrange> auto && r formulation. I need to follow up with lwgchair and maybe with Barry to correct the draft NB comments.