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

release/19.x: [libc++] Fix rejects-valid in std::span copy construction (#104500) #104603

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Aug 16, 2024

Backport 99696b3

Requested by: @ldionne

@llvmbot llvmbot requested a review from a team as a code owner August 16, 2024 15:14
@llvmbot llvmbot added this to the LLVM 19.X Release milestone Aug 16, 2024
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 16, 2024
@llvmbot
Copy link
Member Author

llvmbot commented Aug 16, 2024

@llvm/pr-subscribers-libcxx

Author: None (llvmbot)

Changes

Backport 99696b3

Requested by: @ldionne


Full diff: https://github.com/llvm/llvm-project/pull/104603.diff

2 Files Affected:

  • (modified) libcxx/include/span (+1-1)
  • (modified) libcxx/test/std/containers/views/views.span/span.cons/copy.pass.cpp (+85-41)
diff --git a/libcxx/include/span b/libcxx/include/span
index 60d76d830f0f31..da631cdc3f90e6 100644
--- a/libcxx/include/span
+++ b/libcxx/include/span
@@ -206,10 +206,10 @@ struct __is_std_span<span<_Tp, _Sz>> : true_type {};
 
 template <class _Range, class _ElementType>
 concept __span_compatible_range =
+    !__is_std_span<remove_cvref_t<_Range>>::value &&                //
     ranges::contiguous_range<_Range> &&                             //
     ranges::sized_range<_Range> &&                                  //
     (ranges::borrowed_range<_Range> || is_const_v<_ElementType>) && //
-    !__is_std_span<remove_cvref_t<_Range>>::value &&                //
     !__is_std_array<remove_cvref_t<_Range>>::value &&               //
     !is_array_v<remove_cvref_t<_Range>> &&                          //
     is_convertible_v<remove_reference_t<ranges::range_reference_t<_Range>> (*)[], _ElementType (*)[]>;
diff --git a/libcxx/test/std/containers/views/views.span/span.cons/copy.pass.cpp b/libcxx/test/std/containers/views/views.span/span.cons/copy.pass.cpp
index 28f13e122ddc5e..d3990fd60a459a 100644
--- a/libcxx/test/std/containers/views/views.span/span.cons/copy.pass.cpp
+++ b/libcxx/test/std/containers/views/views.span/span.cons/copy.pass.cpp
@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
+
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 
 // <span>
@@ -14,58 +15,101 @@
 #include <span>
 #include <cassert>
 #include <string>
+#include <utility>
 
 #include "test_macros.h"
 
-template <typename T>
-constexpr bool doCopy(const T &rhs)
-{
-    ASSERT_NOEXCEPT(T{rhs});
-    T lhs{rhs};
-    return lhs.data() == rhs.data()
-     &&    lhs.size() == rhs.size();
-}
+template <class T>
+constexpr void test() {
+  ASSERT_NOEXCEPT(std::span<T>(std::declval<std::span<T> const&>()));
+  ASSERT_NOEXCEPT(std::span<T>{std::declval<std::span<T> const&>()});
 
-struct A{};
-
-template <typename T>
-void testCV ()
-{
-    int  arr[] = {1,2,3};
-    assert((doCopy(std::span<T>  ()          )));
-    assert((doCopy(std::span<T,0>()          )));
-    assert((doCopy(std::span<T>  (&arr[0], 1))));
-    assert((doCopy(std::span<T,1>(&arr[0], 1))));
-    assert((doCopy(std::span<T>  (&arr[0], 2))));
-    assert((doCopy(std::span<T,2>(&arr[0], 2))));
+  // dynamic_extent
+  {
+    std::span<T> x;
+    std::span<T> copy(x);
+    assert(copy.data() == x.data());
+    assert(copy.size() == x.size());
+  }
+  {
+    T array[3] = {};
+    std::span<T> x(array, 3);
+    std::span<T> copy(x);
+    assert(copy.data() == array);
+    assert(copy.size() == 3);
+  }
+  {
+    T array[3] = {};
+    std::span<T> x(array, 2);
+    std::span<T> copy(x);
+    assert(copy.data() == array);
+    assert(copy.size() == 2);
+  }
+
+  // static extent
+  {
+    std::span<T, 0> x;
+    std::span<T, 0> copy(x);
+    assert(copy.data() == x.data());
+    assert(copy.size() == x.size());
+  }
+  {
+    T array[3] = {};
+    std::span<T, 3> x(array);
+    std::span<T, 3> copy(x);
+    assert(copy.data() == array);
+    assert(copy.size() == 3);
+  }
+  {
+    T array[2] = {};
+    std::span<T, 2> x(array);
+    std::span<T, 2> copy(x);
+    assert(copy.data() == array);
+    assert(copy.size() == 2);
+  }
 }
 
+struct Foo {};
+
+constexpr bool test_all() {
+  test<int>();
+  test<const int>();
+  test<volatile int>();
+  test<const volatile int>();
 
-int main(int, char**)
-{
-    constexpr int carr[] = {1,2,3};
+  test<long>();
+  test<const long>();
+  test<volatile long>();
+  test<const volatile long>();
 
-    static_assert(doCopy(std::span<      int>  ()),            "");
-    static_assert(doCopy(std::span<      int,0>()),            "");
-    static_assert(doCopy(std::span<const int>  (&carr[0], 1)), "");
-    static_assert(doCopy(std::span<const int,1>(&carr[0], 1)), "");
-    static_assert(doCopy(std::span<const int>  (&carr[0], 2)), "");
-    static_assert(doCopy(std::span<const int,2>(&carr[0], 2)), "");
+  test<double>();
+  test<const double>();
+  test<volatile double>();
+  test<const volatile double>();
 
-    static_assert(doCopy(std::span<long>()),   "");
-    static_assert(doCopy(std::span<double>()), "");
-    static_assert(doCopy(std::span<A>()),      "");
+  // Note: Can't test non-fundamental types with volatile because we require `T*` to be indirectly_readable,
+  //       which isn't the case when T is volatile.
+  test<Foo>();
+  test<const Foo>();
 
-    std::string s;
-    assert(doCopy(std::span<std::string>   ()     ));
-    assert(doCopy(std::span<std::string, 0>()     ));
-    assert(doCopy(std::span<std::string>   (&s, 1)));
-    assert(doCopy(std::span<std::string, 1>(&s, 1)));
+  test<std::string>();
+  test<const std::string>();
+
+  // Regression test for https://github.com/llvm/llvm-project/issues/104496
+  {
+    struct Incomplete;
+    std::span<Incomplete> x;
+    std::span<Incomplete> copy(x);
+    assert(copy.data() == x.data());
+    assert(copy.size() == x.size());
+  }
+
+  return true;
+}
 
-    testCV<               int>();
-    testCV<const          int>();
-    testCV<      volatile int>();
-    testCV<const volatile int>();
+int main(int, char**) {
+  test_all();
+  static_assert(test_all());
 
   return 0;
 }

Trying to copy-construct a std::span from another std::span holding an
incomplete type would fail as we evaluate the SFINAE for the range-based
constructor. The problem was that we checked for __is_std_span after
checking for the range being a contiguous_range, which hard-errored
because of arithmetic on a pointer to incomplete type.

As a drive-by, refactor the whole test and format it.

Fixes llvm#104496

(cherry picked from commit 99696b3)
@tru tru merged commit 90f2d48 into llvm:release/19.x Aug 19, 2024
9 of 10 checks passed
Copy link

@ldionne (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
Development

Successfully merging this pull request may close these issues.

3 participants