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

Is an array type a valid iterator value type? #183

Closed
CaseyCarter opened this issue Sep 13, 2016 · 6 comments
Closed

Is an array type a valid iterator value type? #183

CaseyCarter opened this issue Sep 13, 2016 · 6 comments

Comments

@CaseyCarter
Copy link
Collaborator

CaseyCarter commented Sep 13, 2016

From microsoft/Range-V3-VS2015#9, range-v3 rejects:

auto vec = std::vector<std::string>{{"a"}, {"b"}, {"c"}};
auto it = ranges::find(vec, "b");

IndirectCallableRelation<equal_to, projected</*...*/>, const char(*)[2]>() is not satisfied because Readable<const char(*)[2]>() is not satisfied because both of these two requirements:

// Experimental additional tests. If nothing else, this is a good workout
// for the common_reference code.
concepts::model_of<Same, common_reference_t<reference_t<I>, value_t<I>>, value_t<I>>(),
concepts::model_of<Same, common_reference_t<rvalue_reference_t<I>, value_t<I>>, value_t<I>>()

fail. (common_reference_t</*...*/> in both cases is const char*.) More generally, all of the algorithms that have a const T& parameter which is constrained via an Indirect concept as if using a const T* iterator will fail similarly when T is an array type.

Options:

  1. Allow array types as value types. (Disclaimer: this contradicts my intuition that is_same_v<T, decay_t<T>> holds for value types, so I can't really present this option without some negative bias.) I think this choice weakens the model due to the lack of regularity of array types.
  2. Don't use the IndirectFoo</*...*/, const T*>() trick to constrain algorithms that accept const T&. This may not be as messy as it seems - all of the affected algorithms use IndirectCallableRelation or its aliases IndirectlyComparable and IndirectStrictWeakOrder.
@CaseyCarter
Copy link
Collaborator Author

all of the affected algorithms

For reference: find, count, search_n, remove, remove_copy, lower_bound, upper_bound, equal_range, binary_search, min, max, minmax.

@ericniebler
Copy link
Owner

ericniebler commented Oct 20, 2016

Allow array types as value types. (Disclaimer: this contradicts my intuition that is_same_v<T, decay_t<T>> holds for value types

I agree. Array types are bogus value types.

// Experimental additional tests. If nothing else, this is a good workout
// for the common_reference code.
concepts::model_of<Same, common_reference_t<reference_t<I>, value_t<I>>, value_t<I>>(),
concepts::model_of<Same, common_reference_t<rvalue_reference_t<I>, value_t<I>>, value_t<I>>()

We can probably just remove these sanity checks from range-v3, cmcstl2, and the Ranges TS. I added them to keep myself honest when I was designing and implementing proxy iterator support. Now they only serve to make compile times worse. Holler if you see a reason to keep them.

Don't use the IndirectFoo</*...*/, const T*>() trick to constrain algorithms that accept const T&. This may not be as messy as it seems - all of the affected algorithms use IndirectCallableRelation or its aliases IndirectlyComparable and IndirectStrictWeakOrder.

The IndirectFoo</*...*/, const T*>() trick is a hack, and I wouldn't shed a tear to see it go. I'm somewhat surprised it lasted this long. We can just replace:

IndirectFoo<Fn, projected<I, Proj>, const T*>()

with

Foo<Fn, indirect_result_of_t<Proj&(I)>, const T&>()

A bit chattier, but so be it.

The larger issue is what to do about iterators with these strange (e.g. array, abstract, function) value types? See ericniebler/range-v3#474.

@ericniebler
Copy link
Owner

See ericniebler/range-v3@5ae0a14.

We can just replace:

IndirectFoo<Fn, projected<I, Proj>, const T*>()

with

Foo<Fn, indirect_result_of_t<Proj&(I)>, const T&>()

This isn't quite right for proxy iterators, actually. This would be closer:

Foo<Fn, result_of_t<Proj&(reference_t<I>)>, const T&>() &&
Foo<Fn, result_of_t<Proj&(value_type_t<I>)>, const T&>()

At least, it would make sense to go through the effected algorithms and see where the extra latitude would be useful.

@ericniebler
Copy link
Owner

Options:

  1. Allow array types as value types. (Disclaimer: this contradicts my intuition that is_same_v<T, decay_t<T>> holds for value types, so I can't really present this option without some negative bias.) I think this choice weakens the model due to the lack of regularity of array types.

So, should we be enforcing your intuition?

  1. Don't use the IndirectFoo</*...*/, const T*>() trick to constrain algorithms that accept const T&. This may not be as messy as it seems - all of the affected algorithms use IndirectCallableRelation or its aliases IndirectlyComparable and IndirectStrictWeakOrder.

Given that the problem is "fixed" by (a) removing the extra superfluous common_reference checks in Readable, and (b) removing the Readable requirement on the type of i++ for InputIterators, do you still want to drop the IndirectFoo</*...*/, const T*>() trick?

I'm now firmly back on the fence about this one given that proxy iterators make dropping the trick rather messy.

@CaseyCarter
Copy link
Collaborator Author

I've never found the "trick" offensive; I'm happy to keep it around now that it works with array types.

@ericniebler
Copy link
Owner

Closing. This question is resolved in the affirmative (arrays are valid value types) by issues #236 and #239.

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