[libc++] Addresses LWG3358

[Mordante]

Summary

LWG3358 §[span.cons] is mistaken that to_address can throw

Since last - first has to throw tests are added to make sure this always
happens.

Depends on D142808

[philnik]

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

125

Why not just a try/catch like we normally do?

[Mordante]

Rebased.

[Mordante]

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

125

I added new macros to make this simpler in D142808, this is based on the macros in rapid-cxx-test.h which we have, but they seem quite unknown.

[Mordante]

Adds missing includes.

[philnik]

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

125

TBH I don't think this is a readability improvement. This just makes it harder to see what's going on.

[ldionne]

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

125

Yeah, I must agree that seeing it in use here it isn't super easy to read. This would be the equivalent code right?

try {
  std::span<int> s{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}});
  assert(false);
} catch (int i) {
  assert(i == 42);
}

I think the part that gets confusing here is probably the fact that we have a predicate that tests the content of the exception. I wonder how useful that is in practice. Otherwise, this could look like TEST_THROWS(int, []{ statements... }), which IMO is easy to read.

[Mordante]

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

125

Yeah, I must agree that seeing it in use here it isn't super easy to read. This would be the equivalent code right?

Any suggestions how to make this macro more readable?

try {
  std::span<int> s{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}});
  assert(false);
} catch (int i) {
  assert(i == 42);
}

Not this is not equivalent, if "hello world" is thrown, the test passes. There needs to be a catch (...).

I think the part that gets confusing here is probably the fact that we have a predicate that tests the content of the exception. I wonder how useful that is in practice. Otherwise, this could look like TEST_THROWS(int, []{ statements... }), which IMO is easy to read.

We indeed have a macro for that case too.

For format tests I find it very useful to test the contents of what(), there I use it to validate the expected error condition is triggered. The format library throws at a lot of places and always the same type. On occasion I've made errors in the format string which resulted in an exception. There it was easy to validate the test was wrong.

[ldionne]

libcxx/include/span

240–246

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

125

Just thinking out loud:

#ifndef TEST_HAS_NO_EXCEPTIONS
assert(42 == thrown_exception<int>([]{
  std::span<int> s{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}};
  (void)s;
}));
#endif

I think there is more benefit to this macro when there are multiple tests back-to-back that use it. Then you do the mental work of figuring out what the macro does once, and you read it mechanically multiple times. In this case here though, there's only two assertions and none elsewhere in the file so the benefit feels less clear to me.

I don't want to block this patch on this though. Feel free to land but let's give some thought to how we can make TEST_VALIDATE_EXCEPTION better generally speaking.

[Mordante]

libcxx/include/span

240–246

I kept the comment, I prefer to plainly cite the Standard, since that is the source of truth.
I agree the other proposed changes are an improvement, so I applied them.

libcxx/docs/Status/Cxx20Issues.csv

 "`3348 <https://wg21.link/LWG3348>`__","``__cpp_lib_unwrap_ref``\  in wrong header","Prague","|Complete|","12.0"
 "`3349 <https://wg21.link/LWG3349>`__","Missing ``__cpp_lib_constexpr_complex``\  for P0415R1","Prague","|Complete|","16.0"
 "`3350 <https://wg21.link/LWG3350>`__","Simplify return type of ``lexicographical_compare_three_way``\ ","Prague","|Complete|","17.0","|spaceship|"
 "`3351 <https://wg21.link/LWG3351>`__","``ranges::enable_safe_range``\  should not be constrained","Prague","|Complete|","15.0","|ranges|"
 "`3352 <https://wg21.link/LWG3352>`__","``strong_equality``\  isn't a thing","Prague","|Nothing To Do|","","|spaceship|"
 "`3354 <https://wg21.link/LWG3354>`__","``has_strong_structural_equality``\  has a meaningless definition","Prague","|Nothing To Do|","","|spaceship|"
 "`3355 <https://wg21.link/LWG3355>`__","The memory algorithms should support move-only input iterators introduced by P1207","Prague","|Complete|","15.0","|ranges|"
 "`3356 <https://wg21.link/LWG3356>`__","``__cpp_lib_nothrow_convertible``\  should be ``__cpp_lib_is_nothrow_convertible``\ ","Prague","|Complete|","12.0"
-"`3358 <https://wg21.link/LWG3358>`__","|sect|\ [span.cons] is mistaken that ``to_address``\  can throw","Prague","",""
+"`3358 <https://wg21.link/LWG3358>`__","|sect|\ [span.cons] is mistaken that ``to_address``\  can throw","Prague","|Complete|","17.0"
 "`3359 <https://wg21.link/LWG3359>`__","``<chrono>``\  leap second support should allow for negative leap seconds","Prague","","","|chrono|"
 "`3360 <https://wg21.link/LWG3360>`__","``three_way_comparable_with``\  is inconsistent with similar concepts","Prague","|Nothing To Do|","","|spaceship|"
 "`3362 <https://wg21.link/LWG3362>`__","Strike ``stop_source``\ 's ``operator!=``\ ","Prague","",""
 "`3363 <https://wg21.link/LWG3363>`__","``drop_while_view``\  should opt-out of ``sized_range``\ ","Prague","|Nothing To Do|","","|ranges|"
 "`3364 <https://wg21.link/LWG3364>`__","Initialize data members of ranges and their iterators","Prague","|Complete|","16.0","|ranges|"
 "`3367 <https://wg21.link/LWG3367>`__","Integer-class conversions should not throw","Prague","|Nothing To Do|",""
 "`3369 <https://wg21.link/LWG3369>`__","``span``\ 's deduction-guide for built-in arrays doesn't work","Prague","|Complete|","14.0"
 "`3371 <https://wg21.link/LWG3371>`__","``visit_format_arg``\  and ``make_format_args``\  are not hidden friends","Prague","|Complete|","14.0","|format|"

libcxx/include/span

// [span.cons], span constructors, copy, assignment, and destructor
     constexpr explicit span(_It __first, size_type __count)
         : __data_{_VSTD::to_address(__first)} {
       (void)__count;
       _LIBCPP_ASSERT(_Extent == __count, "size mismatch in span's constructor (iterator, len)");
     }
 
     template <__span_compatible_iterator<element_type> _It, __span_compatible_sentinel_for<_It> _End>
     _LIBCPP_INLINE_VISIBILITY
     constexpr explicit span(_It __first, _End __last) : __data_{_VSTD::to_address(__first)} {
-      (void)__last;
-      _LIBCPP_ASSERT((__last - __first >= 0), "invalid range in span's constructor (iterator, sentinel)");
-      _LIBCPP_ASSERT(__last - __first == _Extent,
-                     "invalid range in span's constructor (iterator, sentinel): last - first != extent");
+      // [span.cons]/10
+      // Throws: When and what last - first throws.
+      [[maybe_unused]] auto __dist = __last - __first;
+      _LIBCPP_ASSERT(__dist >= 0, "invalid range in span's constructor (iterator, sentinel)");
+      _LIBCPP_ASSERT(
+          __dist == _Extent, "invalid range in span's constructor (iterator, sentinel): last - first != extent");
     }
 
     _LIBCPP_INLINE_VISIBILITY constexpr span(type_identity_t<element_type> (&__arr)[_Extent]) noexcept : __data_{__arr} {}
 
     template <__span_array_convertible<element_type> _OtherElementType>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(array<_OtherElementType, _Extent>& __arr) noexcept : __data_{__arr.data()} {}
 

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

 //===---------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===---------------------------------------------------------------------===//
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 
 // <span>
 
 // template <class It, class End>
 // constexpr explicit(Extent != dynamic_extent) span(It first, End last);
 // Requires: [first, last) shall be a valid range.
 //   If Extent is not equal to dynamic_extent, then last - first shall be equal to Extent.
-//
+// Throws: When and what last - first throws.
 
+#include <array>
 #include <span>
 #include <cassert>
+#include <utility>
 
+#include "assert_macros.h"
 #include "test_iterators.h"
+#include "test_macros.h"
 
 template <class T, class Sentinel>
 constexpr bool test_ctor() {
   T val[2] = {};
-  auto s1 = std::span<T>(std::begin(val), Sentinel(std::end(val)));
-  auto s2 = std::span<T, 2>(std::begin(val), Sentinel(std::end(val)));
+  auto s1  = std::span<T>(std::begin(val), Sentinel(std::end(val)));
+  auto s2  = std::span<T, 2>(std::begin(val), Sentinel(std::end(val)));
   assert(s1.data() == std::data(val) && s1.size() == std::size(val));
   assert(s2.data() == std::data(val) && s2.size() == std::size(val));
   return true;
 }
 
 template <size_t Extent>
 constexpr void test_constructibility() {
   static_assert(std::is_constructible_v<std::span<int, Extent>, int*, int*>);
constexpr bool test() {
   struct A {};
   assert((test_ctor<int, int*>()));
   assert((test_ctor<int, sized_sentinel<int*>>()));
   assert((test_ctor<A, A*>()));
   assert((test_ctor<A, sized_sentinel<A*>>()));
   return true;
 }
 
+#ifndef TEST_HAS_NO_EXCEPTIONS
+// A stripped down contiguous iterator that throws when using operator-.
+template <class It>
+class throw_operator_minus {
+  It it_;
+
+public:
+  typedef std::contiguous_iterator_tag iterator_category;
+  typedef typename std::iterator_traits<It>::value_type value_type;
+  typedef typename std::iterator_traits<It>::difference_type difference_type;
+  typedef It pointer;
+  typedef typename std::iterator_traits<It>::reference reference;
+  typedef typename std::remove_pointer<It>::type element_type;
+
+  throw_operator_minus() : it_() {}
+  explicit throw_operator_minus(It it) : it_(it) {}
+
+  reference operator*() const { return *it_; }
+  pointer operator->() const { return it_; }
+  reference operator[](difference_type n) const { return it_[n]; }
+
+  throw_operator_minus& operator++() {
+    ++it_;
+    return *this;
+  }
+  throw_operator_minus& operator--() {
+    --it_;
+    return *this;
+  }
+  throw_operator_minus operator++(int) { return throw_operator_minus(it_++); }
+  throw_operator_minus operator--(int) { return throw_operator_minus(it_--); }
+
+  throw_operator_minus& operator+=(difference_type n) {
+    it_ += n;
+    return *this;
+  }
+  throw_operator_minus& operator-=(difference_type n) {
+    it_ -= n;
+    return *this;
+  }
+  friend throw_operator_minus operator+(throw_operator_minus x, difference_type n) {
+    x += n;
+    return x;
+  }
+  friend throw_operator_minus operator+(difference_type n, throw_operator_minus x) {
+    x += n;
+    return x;
+  }
+  friend throw_operator_minus operator-(throw_operator_minus x, difference_type n) {
+    x -= n;
+    return x;
+  }
+  friend difference_type operator-(throw_operator_minus, throw_operator_minus) { throw 42; };
+
+  friend bool operator==(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ == y.it_; }
+  friend bool operator<=>(const throw_operator_minus& x, const throw_operator_minus& y) { return x.it_ <=> y.it_; }
+};
+
+template <class It>
+throw_operator_minus(It) -> throw_operator_minus<It>;
+
+void test_exceptions() {
+  std::array a{42};
+  TEST_VALIDATE_EXCEPTION(
+      int,
+      [](int i) { assert(i == 42); },
+      (std::span<int>{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}}));
+  TEST_VALIDATE_EXCEPTION(
+      int,
+      [](int i) { assert(i == 42); },
+      (std::span<int, 1>{throw_operator_minus{a.begin()}, throw_operator_minus{a.end()}}));
+}
+#endif // TEST_HAS_NO_EXCEPTIONS
+
 int main(int, char**) {
   test();
+#ifndef TEST_HAS_NO_EXCEPTIONS
+  test_exceptions();
+#endif
   static_assert(test());
 
   return 0;
 }