[libcxx-commits] [PATCH] D110503: [libc++] Implement P1394r4 for span: range constructor
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sun Sep 26 10:12:51 PDT 2021
Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/include/span:69-70
constexpr span(const array<value_type, N>& arr) noexcept;
- template <class Container>
- constexpr explicit(Extent != dynamic_extent) span(Container& cont);
- template <class Container>
- constexpr explicit(Extent != dynamic_extent) span(const Container& cont);
+ template <class Range>
+ constexpr span(Range&& range);
constexpr span(const span& other) noexcept = default;
----------------
This ctor should also be conditionally `explicit`, according to https://eel.is/c++draft/span.cons
[p1394r4] is missing all these explicits for some reason, so eel.is (or whatever copy of the working draft) should be our guide.
================
Comment at: libcxx/include/span:179-183
+ typename enable_if<!__is_std_span<remove_cvref_t<_Tp>>::value, nullptr_t>::type,
// is not a specialization of array
- typename enable_if<!__is_std_array<_Tp>::value, nullptr_t>::type,
+ typename enable_if<!__is_std_array<remove_cvref_t<_Tp>>::value, nullptr_t>::type,
// is_array_v<Container> is false,
typename enable_if<!is_array_v<_Tp>, nullptr_t>::type,
----------------
If you're updating these, might as well make them all use e.g.
`enable_if_t<!__is_std_span<remove_cvref_t<_Tp>>::value>,`
================
Comment at: libcxx/include/span:196
+template <class _Range, class _ElementType>
+inline constexpr bool __is_range_compatible =
+ ranges::contiguous_range<_Range> &&
----------------
If we're assuming Ranges+Concepts support (`contiguous_range`), then we can use `concept` instead of `inline constexpr bool`, and make things maybe very slightly easier on the compiler. However, I'm not sure that we //can// assume Concepts support; don't we need some #ifdef guards sprinkled through here? Ditto on the deduction guides that use `ranges::` things.
Also, the name of this should be something like `__is_span_compatible_range`; remember this is directly in namespace `_VSTD` along with everything else in the library, so its name shouldn't be too generic.
================
Comment at: libcxx/include/span:429
+ _LIBCPP_INLINE_VISIBILITY constexpr span(_It __first, _End __last)
+ : __data{to_address(__first)}, __size{static_cast<size_t>(distance(__first, __last))} {}
----------------
ADL-proof: `_VSTD::to_address`, `_VSTD::distance`. Ditto lines 236, 248.
(The LWG clauses of the Standard have a bad, but at least consistent, pattern, of never writing `std::` when it is "obvious." This makes it hard for them to indicate when ADL is actually //intended//, e.g. on `swap`; but any time ADL isn't clearly intended, you should always read their `foo` as `std::foo`.)
I also (always, but especially here) strongly recommend using `()` for direct-initialization, never `{}`. Parens are //always// correct; braces are //sometimes// equivalent to parens and //sometimes// wrong (either because they get hijacked by initializer_list ctors, or because they prohibit narrowing conversions when LWG didn't say to prohibit them).
================
Comment at: libcxx/include/span:445
- template <class _Container>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span( _Container& __c,
- enable_if_t<__is_span_compatible_container<_Container, _Tp>::value, nullptr_t> = nullptr)
- : __data{_VSTD::data(__c)}, __size{(size_type) _VSTD::size(__c)} {}
-
- template <class _Container>
- _LIBCPP_INLINE_VISIBILITY
- constexpr span(const _Container& __c,
- enable_if_t<__is_span_compatible_container<const _Container, _Tp>::value, nullptr_t> = nullptr)
- : __data{_VSTD::data(__c)}, __size{(size_type) _VSTD::size(__c)} {}
-
+ template <class _Range, class = enable_if_t<__is_range_compatible<_Range, element_type>>>
+ _LIBCPP_INLINE_VISIBILITY constexpr explicit span(_Range&& __r) : __data{ranges::data(__r)}, __size{ranges::size(__r)} {
----------------
Slight scope-creep: For consistency with elsewhere, please rewrite the SFINAE as
```
template <class _Range, enable_if_t<__is_range_compatible<_Range, element_type>>* = nullptr>
```
The benefit (in general) is that it's possible to have two overloaded function templates with template-heads
```
template <class _Range, enable_if_t<A>* = nullptr>
template <class _Range, enable_if_t<B>* = nullptr>
```
whereas it is //not// possible to have two overloaded function templates with template-heads
```
template <class _Range, class = enable_if_t<A>>
template <class _Range, class = enable_if_t<B>>
```
(because those would be //one// template with two declarations, with two incompatible defaults for the defaulted parameter).
================
Comment at: libcxx/include/span:574-575
// Deduction guides
+template<class _It, class _End, class = enable_if_t<contiguous_iterator<_It>>>
+ span(_It, _End) -> span<remove_reference_t<iter_reference_t<_It>>>;
+
----------------
I believe we can write this as
```
template<contiguous_iterator _It, class _EndOrSize>
span(_It, _EndOrSize) -> span<remove_reference_t<iter_reference_t<_It>>>;
```
Note the name change to `_EndOrSize`, to match the working draft. (This also self-documents why it's not constrained as `sentinel_for<_It>`: it's because it's not //necessarily// an `_End`; it might be a `_Size` instead, e.g. an `int`.)
Ditto line 586: `template<ranges::contiguous_range _Range>`.
================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_iterator.fail.cpp:48-54
+ std::span< int> s1{std::begin(carr), std::end(carr)}; // expected-error {{no matching constructor for initialization of 'std::span<int>'}}
+ std::span< int> s2{std::begin(varr), std::end(varr)}; // expected-error {{no matching constructor for initialization of 'std::span<int>'}}
+ std::span< int> s3{std::begin(cvarr), std::end(cvarr)}; // expected-error {{no matching constructor for initialization of 'std::span<int>'}}
+ std::span<const int> s4{std::begin(varr), std::end(varr)}; // expected-error {{no matching constructor for initialization of 'std::span<const int>'}}
+ std::span<const int> s5{std::begin(cvarr), std::end(cvarr)}; // expected-error {{no matching constructor for initialization of 'std::span<const int>'}}
+ std::span< volatile int> s6{std::begin(carr), std::end(carr)}; // expected-error {{no matching constructor for initialization of 'std::span<volatile int>'}}
+ std::span< volatile int> s7{std::begin(cvarr), std::end(cvarr)}; // expected-error {{no matching constructor for initialization of 'std::span<volatile int>'}}
----------------
I'd prefer to see these as simple compile-time tests:
```
static_assert( std::is_constructible_v<std::span<int>, int*, int*>);
static_assert(!std::is_constructible_v<std::span<int>, const int*, const int*>);
static_assert(!std::is_constructible_v<std::span<int>, volatile int*, volatile int*>);
static_assert( std::is_constructible_v<std::span<const int>, int*, int*>);
static_assert( std::is_constructible_v<std::span<const int>, const int*, const int*>);
static_assert(!std::is_constructible_v<std::span<const int>, volatile int*, volatile int*>);
static_assert( std::is_constructible_v<std::span<volatile int>, int*, int*>);
static_assert(!std::is_constructible_v<std::span<volatile int>, const int*, const int*>);
static_assert( std::is_constructible_v<std::span<volatile int>, volatile int*, volatile int*>);
```
etc. And then similarly for `std::is_convertible_v` to test the non-explicit ctors.
I think it would be //nice// (but not necessary, and maybe excessively redundant) to put in some realistic tests that use `span` as a parameter-only type in the appropriate way:
```
void f1(std::span<int>) {}
void f2(std::span<const int>) {}
void fs1(std::span<int, 3>) {}
void fs2(std::span<const int, 3>) {}
int main() {
std::vector<int> v = {1,2,3};
f1(v);
f2(v);
f2(std::vector<int>{1,2,3});
f1({v.data(), v.size()});
f2({v.data(), v.size()});
fs1(std::span{v.data(), v.size()}); // this works, right?
fs2(std::span{v.data(), v.size()}); // this works, right?
}
```
and so on, just to touch all the relevant real-world use cases.
================
Comment at: libcxx/test/std/containers/views/span.cons/iterator_len.pass.cpp:80-82
+ return
+ s1.data() == &val[0] && s1.size() == 2
+ && s2.data() == &val[0] && s2.size() == 2;
----------------
```
T val[2] = {};
auto s1 = std::span<T>(val, 2);
auto s2 = std::span<T, 2>(val, 2);
assert(s1.data() == &val[0] && s1.size() == 2);
assert(s2.data() == &val[0] && s2.size() == 2);
return true;
```
The big things here are that you can use `assert` in constexpr functions, and that you //don't// mark variables `constexpr` inside constexpr functions. I assumed that the `const` in `const T` was not significant to what you were trying to test; otherwise you could bring it back.
Probably you can collapse `testConstexprSpan` and `testRuntimeSpan` into the same function, now.
================
Comment at: libcxx/test/std/containers/views/span.cons/range.fail.cpp:48-49
+
+template <class T>
+inline constexpr bool std::ranges::enable_borrowed_range<DisabledBorrowedRange<T>> = false;
+
----------------
This is redundant with the primary template (thus `DisabledBorrowedRange` is redundant with `IsARange`). Did you mean to try with a type that //enabled// borrowed-range for itself? I'd suggest just using `string_view` for that.
================
Comment at: libcxx/test/std/containers/views/span.cons/range.fail.cpp:59-61
+ std::span<const int> s3{std::deque<int>()}; // expected-error {{no matching constructor for initialization of 'std::span<const int>'}}
+ std::span<const int> s4{std::list<int>()}; // expected-error {{no matching constructor for initialization of 'std::span<const int>'}}
+ std::span<const int> s5{std::forward_list<int>()}; // expected-error {{no matching constructor for initialization of 'std::span<const int>'}}
----------------
No need to drag in all three of these; one will suffice. The relevant property is "being a range, but //not// being a contiguous range"... actually, waitaminute... these containers //also// are not borrowed ranges! So of course lines 59-61 won't compile, for a whole host of reasons. I don't think there's any reason to have lines 59-61 at all, now.
However, you might test a "borrowed but non-contiguous" rvalue range, and a "lvalue but non-contiguous" non-borrowed range. Again I recommend doing this as a compile-time static_assert, something like:
```
static_assert( std::is_constructible_v<std::span<int>, std::vector<int>&>); // non-borrowed lvalue
static_assert(!std::is_constructible_v<std::span<int>, std::vector<int>&&>); // non-borrowed rvalue
static_assert(!std::is_constructible_v<std::span<int>, std::deque<int>&>); // non-contiguous lvalue
static_assert(!std::is_constructible_v<std::span<int>, std::subrange<std::deque<int>::iterator, std::deque<int>::iterator>&&>); // non-contiguous borrowed rvalue
```
================
Comment at: libcxx/test/std/containers/views/span.cons/simple_range.h:4
+template <class T, size_t Size>
+struct SimpleRange {
+ constexpr T *data() {
----------------
Once you convert `range.fail.cpp` to a bunch of static_asserts, you'll be using `SimpleRange` in only one file, and then it won't need its own header, so this comment will be moot.
If this class isn't scoped to a single test, then it should have a descriptive name like `SimpleContiguousNonborrowedRange`... which is just `std::vector`, isn't it? What's the point of making our own type here at all?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110503/new/
https://reviews.llvm.org/D110503
More information about the libcxx-commits
mailing list