[libc++] Disallow dynamic -> static span conversions

[ldionne]

Summary

[libc++] Disallow dynamic -> static span conversions

This change disallows dynamic to static span conversions. This complies
with the standard, which lists the following constaint in
views.span#span.cons-20.1 [1]:

Extent == dynamic_extent || Extent == OtherExtent

Thus this should fail if Extent != dynamic_extent && Extent !=
OtherExtent, which is the case when trying to construct a static span
from a dynamic span.

[1] https://eel.is/c++draft/views.span#span.cons-20.1

[jdoerrie]

Formatting

[jdoerrie]

Full patch

[jdoerrie]

Friendly Ping :)

[jorgbrown]

+1

Thanks also for fixing the return statement of testConversionSpan() in span.pass.cpp

[jdoerrie]

Thanks Jorg! Given that the status is still "Needs Review", I assume I also still need an LGTM from Marshall, Louis or Eric, right?

[mclow.lists]

libcxx/test/std/containers/views/span.cons/span.fail.cpp

78

Ok. The comment here is wrong; this is testing dynamic -> static.

However, why are you removing these (failing) tests?

libcxx/test/std/containers/views/span.cons/span.pass.cpp

65

These cases (on the other hand) should be moved to a failure test.

80

Please line these up like the other ones were. Makes it easy to see copy-pasta errors:

return s1.data() == nullptr && s1.size() == 0 
    && s2.data() == nullptr && s2.size() == 0
    && s3.data() == nullptr && s3.size() == 0;

113

same comment as above re: alignment.

[jdoerrie]

Addressed Marshall's comments

libcxx/test/std/containers/views/span.cons/span.fail.cpp

78

Considering that Extent == dynamic_extent || Extent == OtherExtent should be checked first I felt these particular tests distracted from the actual root cause of the error. I added now a new section that performs explicit Extent checks.

libcxx/test/std/containers/views/span.cons/span.pass.cpp

80

FWIW my formatting was performed by clang-format. Do you think it's worth wrapping this block in

// clang-format off
...
// clang-format on

so that this doesn't regress in the future?

[jdoerrie]

Friendly Ping

[miscco]

I believe this is superseded by the implementation of P1976R2 in D74577

[jdoerrie]

Oh interesting! Yes, it looks like you are right.

[ldionne]

In D69520#1878360, @miscco wrote:

I believe this is superseded by the implementation of P1976R2 in D74577

@miscco Can you please look at the tests in this patch and salvage any test you might have omitted from your own patch?

@jdoerrie I'm sorry there was some confusion and duplicate work, but thanks for your contribution nonetheless!

libcxx/include/span

// [span.cons], span constructors, copy, assignment, and destructor
     template <class _OtherElementType>
     _LIBCPP_INLINE_VISIBILITY
         constexpr span(const span<_OtherElementType, _Extent>& __other,
                        enable_if_t<
                           is_convertible_v<_OtherElementType(*)[], element_type (*)[]>,
                           nullptr_t> = nullptr)
         : __data{__other.data()} {}
 
-    template <class _OtherElementType>
-    _LIBCPP_INLINE_VISIBILITY
-        constexpr span(const span<_OtherElementType, dynamic_extent>& __other,
-                       enable_if_t<
-                          is_convertible_v<_OtherElementType(*)[], element_type (*)[]>,
-                          nullptr_t> = nullptr) noexcept
-        : __data{__other.data()} { _LIBCPP_ASSERT(_Extent == __other.size(), "size mismatch in span's constructor (other span)"); }
-
-
 //  ~span() noexcept = default;
 
     template <size_t _Count>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span<element_type, _Count> first() const noexcept
     {
         static_assert(_Count <= _Extent, "Count out of range in span::first()");
         return {data(), _Count};

libcxx/test/std/containers/views/span.cons/span.fail.cpp

//  Try to remove const and/or volatile (static -> dynamic)
     std::span<               int> s3{cvsp0}; // expected-error {{no matching constructor for initialization of 'std::span<int>'}}
 
     std::span<const          int> s4{ vsp0}; // expected-error {{no matching constructor for initialization of 'std::span<const int>'}}
     std::span<const          int> s5{cvsp0}; // expected-error {{no matching constructor for initialization of 'std::span<const int>'}}
 
     std::span<      volatile int> s6{ csp0}; // expected-error {{no matching constructor for initialization of 'std::span<volatile int>'}}
     std::span<      volatile int> s7{cvsp0}; // expected-error {{no matching constructor for initialization of 'std::span<volatile int>'}}
     }
+}
 
-//  Try to remove const and/or volatile (static -> static)
-    {
-    std::span<               int, 0> s1{ csp}; // expected-error {{no matching constructor for initialization of 'std::span<int, 0>'}}
-    std::span<               int, 0> s2{ vsp}; // expected-error {{no matching constructor for initialization of 'std::span<int, 0>'}}
-    std::span<               int, 0> s3{cvsp}; // expected-error {{no matching constructor for initialization of 'std::span<int, 0>'}}
+void checkExtent ()
+    std::span<int>    spd;
+    std::span<int, 0> sp0;
+    std::span<int, 1> sp1{nullptr, 1};
+    std::span<int, 2> sp2{nullptr, 2};
 
-    std::span<const          int, 0> s4{ vsp}; // expected-error {{no matching constructor for initialization of 'std::span<const int, 0>'}}
-    std::span<const          int, 0> s5{cvsp}; // expected-error {{no matching constructor for initialization of 'std::span<const int, 0>'}}
+//  Try to construct static span with extent 0 from span with different extent
+    {
+    std::span<int, 0> s1{spd}; // expected-error {{no matching constructor for initialization of 'std::span<int, 0>'}}
+    std::span<int, 0> s2{sp1}; // expected-error {{no matching constructor for initialization of 'std::span<int, 0>'}}
+    std::span<int, 0> s3{sp2}; // expected-error {{no matching constructor for initialization of 'std::span<int, 0>'}}
+    }
 
-    std::span<      volatile int, 0> s6{ csp}; // expected-error {{no matching constructor for initialization of 'std::span<volatile int, 0>'}}
-    std::span<      volatile int, 0> s7{cvsp}; // expected-error {{no matching constructor for initialization of 'std::span<volatile int, 0>'}}
+//  Try to construct static span with extent 1 from span with different extent
+    {
+    std::span<int, 1> s1{spd}; // expected-error {{no matching constructor for initialization of 'std::span<int, 1>'}}
+    std::span<int, 1> s2{sp0}; // expected-error {{no matching constructor for initialization of 'std::span<int, 1>'}}
+    std::span<int, 1> s3{sp2}; // expected-error {{no matching constructor for initialization of 'std::span<int, 1>'}}
+    }
+
+//  Try to construct static span with extent 2 from span with different extent
+    {
+    std::span<int, 2> s1{spd}; // expected-error {{no matching constructor for initialization of 'std::span<int, 2>'}}
+    std::span<int, 2> s2{sp0}; // expected-error {{no matching constructor for initialization of 'std::span<int, 2>'}}
+    std::span<int, 2> s3{sp1}; // expected-error {{no matching constructor for initialization of 'std::span<int, 2>'}}
     }
 }
 
 int main(int, char**)
 {
     std::span<int>      sp;
     std::span<int, 0>   sp0;
 
     std::span<float> s1{sp};    // expected-error {{no matching constructor for initialization of 'std::span<float>'}}
     std::span<float> s2{sp0};   // expected-error {{no matching constructor for initialization of 'std::span<float>'}}
     std::span<float, 0> s3{sp}; // expected-error {{no matching constructor for initialization of 'std::span<float, 0>'}}
     std::span<float, 0> s4{sp0};    // expected-error {{no matching constructor for initialization of 'std::span<float, 0>'}}
 
     checkCV();
+    checkExtent();
 
   return 0;
 }

libcxx/test/std/containers/views/span.cons/span.pass.cpp

 //  static -> dynamic
     {
         std::span<const          int> s1{  sp0};    // a span<const          int> pointing at int.
         std::span<      volatile int> s2{  sp0};    // a span<      volatile int> pointing at int.
         std::span<const volatile int> s3{  sp0};    // a span<const volatile int> pointing at int.
         std::span<const volatile int> s4{ vsp0};    // a span<const volatile int> pointing at volatile int.
         assert(s1.size() + s2.size() + s3.size() + s4.size() == 0);
     }
-
-//  dynamic -> static
-    {
-        std::span<const          int, 0> s1{  sp};  // a span<const          int> pointing at int.
-        std::span<      volatile int, 0> s2{  sp};  // a span<      volatile int> pointing at int.
-        std::span<const volatile int, 0> s3{  sp};  // a span<const volatile int> pointing at int.
-        std::span<const volatile int, 0> s4{ vsp};  // a span<const volatile int> pointing at volatile int.
-        assert(s1.size() + s2.size() + s3.size() + s4.size() == 0);
-    }
 }
 
 
 template <typename T>
 constexpr bool testConstexprSpan()
 {
-    std::span<T>    s0{};
-    std::span<T, 0> s1(s0); // dynamic -> static
-    std::span<T>    s2(s1); // static -> dynamic
-    ASSERT_NOEXCEPT(std::span<T>   {s0});
-    ASSERT_NOEXCEPT(std::span<T, 0>{s1});
-    ASSERT_NOEXCEPT(std::span<T>   {s1});
-    ASSERT_NOEXCEPT(std::span<T, 0>{s0});
-
-    return
-        s1.data() == nullptr && s1.size() == 0
-    &&  s2.data() == nullptr && s2.size() == 0;
+  std::span<T> s0d{};
+  std::span<T, 0> s0s{};
+  std::span<T> s1(s0d);    // dynamic -> dynamic
+  std::span<T, 0> s2(s0s); // static -> static
+  std::span<T> s3(s2);     // static -> dynamic
+  ASSERT_NOEXCEPT(std::span<T>{s0d});
+  ASSERT_NOEXCEPT(std::span<T, 0>{s0s});
+  ASSERT_NOEXCEPT(std::span<T>{s0s});
+
+  return s1.data() == nullptr && s1.size() == 0
+      && s2.data() == nullptr && s2.size() == 0
+      && s3.data() == nullptr && s3.size() == 0;
 }
 
 
 template <typename T>
 void testRuntimeSpan()
 {
-    std::span<T>    s0{};
-    std::span<T, 0> s1(s0); // dynamic -> static
-    std::span<T>    s2(s1); // static -> dynamic
-    ASSERT_NOEXCEPT(std::span<T>   {s0});
-    ASSERT_NOEXCEPT(std::span<T, 0>{s1});
-    ASSERT_NOEXCEPT(std::span<T>   {s1});
-    ASSERT_NOEXCEPT(std::span<T, 0>{s0});
+  std::span<T> s0d{};
+  std::span<T, 0> s0s{};
+  std::span<T> s1(s0d);    // dynamic -> dynamic
+  std::span<T, 0> s2(s0s); // static -> static
+  std::span<T> s3(s2);     // static -> dynamic
+  ASSERT_NOEXCEPT(std::span<T>{s0d});
+  ASSERT_NOEXCEPT(std::span<T, 0>{s0s});
+  ASSERT_NOEXCEPT(std::span<T>{s0s});
 
-    assert(s1.data() == nullptr && s1.size() == 0);
-    assert(s2.data() == nullptr && s2.size() == 0);
+  assert(s3.data() == nullptr && s3.size() == 0);
 }
 
 
 template <typename Dest, typename Src>
 bool testConversionSpan()
 {
     static_assert(std::is_convertible_v<Src(*)[], Dest(*)[]>, "Bad input types to 'testConversionSpan");
     std::span<Src>    s0d{};
-    std::span<Src>    s0s{};
-    std::span<Dest, 0> s1(s0d); // dynamic -> static
-    std::span<Dest>    s2(s0s); // static -> dynamic
-        s1.data() == nullptr && s1.size() == 0
-    &&  s2.data() == nullptr && s2.size() == 0;
+    std::span<Src, 0> s0s{};
+    std::span<Dest> s1(s0d);    // dynamic -> dynamic
+    std::span<Dest, 0> s2(s0s); // static -> static
+    std::span<Dest> s3(s0d);    // static -> dynamic
+    return s1.data() == nullptr && s1.size() == 0
+        && s2.data() == nullptr && s2.size() == 0
+        && s3.data() == nullptr && s3.size() == 0;
 }
 
 struct A{};
 
 int main(int, char**)
 {
     static_assert(testConstexprSpan<int>(),    "");
     static_assert(testConstexprSpan<long>(),   "");