[libcxx-commits] [PATCH] D110503: [libc++] Implement P1394r4 for span: range constructor

[Joe Loser via Phabricator via libcxx-commits]

jloser marked an inline comment as done.
jloser added inline comments.


================
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>'}}
----------------
Quuxplusone wrote:
> 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.
I reworked several tests (itr/itr, itr/len, range) to mostly be compile-time tests that use `std::is_constructible_v` and `std::is_convertible_v` and I like that a lot better!

I did leave `iterator_iterator.fail.cpp` and `iterator_len.fail.cpp` which test the explicit constructors respectively. I don't see how we can use `std::is_convertible_v` since the constructor takes more than one parameter (two in each of these cases), but we only have one template type for the from type in `std::is_convertible_v`. Am I missing something?

As for the idea around adding tests that we can call a function taking a span parameter, I don't immediately see the value-add it has from a coverage perspective. I haven't added them now, but if you can convince me what coverage it adds, I'm happy to add them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110503/new/

https://reviews.llvm.org/D110503