Issues regarding span<T> construction from containers

[bogiord]

Sorry for the long list of issues, but they're all related; I don't think splitting the list into several issues would help.

1. The constructor template taking a const std::array<...>& will never be used, as the T template parameter cannot be deduced, so specializations of this template will never participate in overload resolution. Because of this, construction from const std::array<...> falls back to generic construction from containers (Cont&).

   I'm not sure about the reason for using std::remove_const_t<value_type> as the element type for the std::array parameter, but, anyway, T either goes here or goes away completely.

2. Both constructor templates taking lvalue references to std::array specializations use std::is_convertible<T(*) [], std::remove_const_t<value_type>(*) []>. Some minor issues are that the first one has a redundant typename, and the second one has a typo at the end, so it won't work.

   More importantly, I'm not sure I understand the reason for using std::remove_const_t. Removing const from the top level qualifiers of the array element type (which also removes it from the qualifiers of the array itself) basically ensures that the conversion will only work if T and std::remove_const_t<value_type> are exactly the same type. In order to allow some qualification conversion between the two pointer types, the destination type would have to have const in the qualifiers of the upper levels of its cv-qualification signature.

   I think allowing qualification conversions here would be fine, just like for built-in arrays. I'd use std::is_convertible<T(*) [], value_type(*) []>, coupled with an additional check for std::is_const<value_type> when taking a const std::array<...> (or maybe using some SpanStdArrayTraits to add const to T itself in this case... but this may just be overkill). By the way, I think this additional test should also be added as a static_assert to the constructor template taking Cont& when Cont is_const, to provide a better error message.

3. The deleted constructor taking an rvalue reference should take const std::array<T, N>&&. Otherwise, const rvalues will end up with the constructor template taking const std::array<T, N>& (once it's fixed), which means construction from const rvalues will be allowed.

4. The deleted constructor template taking Cont&& is explicit, which means it will be ignored for copy-initialization - arguably the most common scenario for initializing a span<T>. This means, for example, that copy-initialization from a const container rvalue will work through the constructor template taking Cont& (on MSVC, even non-const ones will work, due to the infamous non-conforming extension).

   Which reminds me: the tests in span_tests.cpp use direct-initialization in most cases. I think there should be at least as many copy-initializations in there, in order to catch issues like the one above.

   Also, it seems that the continuous integration tests don't use CONFIRM_COMPILATION_ERRORS; otherwise, they should have caught this one, which doesn't fail, as explained above.

5. And, slightly off-topic, an issue related to construction from another span<...>: shouldn't the convertibility test be something like std::is_convertible<OtherValueType(*)[], ValueType(*)[]>? I mean, the way it's written now, the test would yield true for double and int (double is convertible to int), but we don't want to view an underlying array of double through a span<int>. On the other hand, qualification conversions should be fine.

[neilmacintosh]

Thanks for the detailed issue report @bogiord.

I took a look at the current source for span and here's what I found:

1. The constructors from std::array definitely participate in overload resolution with their current definition, so I think this is resolved at this point.
2. We no longer support the "convertible to" option for element_type on those constructors. There should now be additional tests that cover all the const-qualified permutations of std::array and its element_type. If you see something you think is missing, feel free to offer a test case that describes it.
3. We no longer delete the rvalue reference constructors.
4. We no longer delete the rvalue reference constructors.
5. We believe the new set of constructors offer sufficient "convertibility". But if you think there are important use cases we are now missing, please feel free to open a new issue and offer them for discussion.

I'll close out this issue now, as I think enough of the span interface has changed that any further discussion probably deserves a fresh thread to avoid confusion. Thanks again for the feedback!