[libc++][NFCI] span: replace enable_if with concepts

[Mordante]

Summary

Several span constructors use enable_if which is verbose. Replace these with
concepts or requires expressions.

[Mordante]

Thanks for working on this! In general I prefer concepts over enable_ifs. Unfortunately some of our supported compilers don't have proper concepts support yet. We could use #ifs to guard against that, however IMO that would make the code unneeded ugly. Alternatively we wait until we only support compilers with concepts support.

@ldionne Do you know when AppleClang 13 will be released and whether it has concepts support?

(I want to discuss this before doing a review.)

[• Quuxplusone]

libcxx/include/span

237

enable_if_t which is verbose and slow

I benchmarked enable_if_t versus _EnableIf versus requires over at https://reviews.llvm.org/D108216#inline-1037983 a couple days ago, and it seemed that requires was about 20% slower than enable_if_t/_EnableIf. What evidence led you to the "and slow" part of your comment?

[jloser]

Tone down wording in commit message with regard to speed of enable_if_t vs requires

[jloser]

libcxx/include/span

237

Sorry, I was a bit aggressive/strong in my commit wording here: I've adjusted that now. I have not benchmarked enable_if_t vs requires in this commit, so I've omitted the speed remark.

It surprises me a bit that requires is that much slower. I'd be interested to see an extension of your benchmark to account for enable_if_t when used as a default template parameter (e.g. does the number of templates make things go "really bad" after some threshold?).

[jloser]

In D109212#2981554, @Mordante wrote:

Thanks for working on this! In general I prefer concepts over enable_ifs. Unfortunately some of our supported compilers don't have proper concepts support yet. We could use #ifs to guard against that, however IMO that would make the code unneeded ugly. Alternatively we wait until we only support compilers with concepts support.

@ldionne Do you know when AppleClang 13 will be released and whether it has concepts support?

(I want to discuss this before doing a review.)

Thanks for some context. I started poking around in span last night as I came across this recent libc++ bug. It made me realize there are several missing constraints for span, which motivated a move to requires clauses first to make things not as verbose in fixing all of the constraints.

[ldionne]

In D109212#2982390, @jloser wrote:

In D109212#2981554, @Mordante wrote:

Thanks for working on this! In general I prefer concepts over enable_ifs. Unfortunately some of our supported compilers don't have proper concepts support yet. We could use #ifs to guard against that, however IMO that would make the code unneeded ugly. Alternatively we wait until we only support compilers with concepts support.

@ldionne Do you know when AppleClang 13 will be released and whether it has concepts support?

(I want to discuss this before doing a review.)

Thanks for some context. I started poking around in span last night as I came across this recent libc++ bug. It made me realize there are several missing constraints for span, which motivated a move to requires clauses first to make things not as verbose in fixing all of the constraints.

Thanks a lot for your interest in fixing this. Since we have been shipping std::span and we do support compilers that don't implement concepts yet, I'd like to push back against this change *for the time being*.

What I suggest is that you implement the changes discussed in https://bugs.llvm.org/show_bug.cgi?id=51443 using enable_if_t, and then we revisit this change as soon as all supported compilers implement concepts, which should be the case within a couple months. When that's the case, this patch will have all my support. Does that seem reasonable?

[jloser]

In D109212#2982777, @ldionne wrote:

In D109212#2982390, @jloser wrote:

In D109212#2981554, @Mordante wrote:

Thanks for working on this! In general I prefer concepts over enable_ifs. Unfortunately some of our supported compilers don't have proper concepts support yet. We could use #ifs to guard against that, however IMO that would make the code unneeded ugly. Alternatively we wait until we only support compilers with concepts support.

@ldionne Do you know when AppleClang 13 will be released and whether it has concepts support?

(I want to discuss this before doing a review.)

Thanks for some context. I started poking around in span last night as I came across this recent libc++ bug. It made me realize there are several missing constraints for span, which motivated a move to requires clauses first to make things not as verbose in fixing all of the constraints.

Thanks a lot for your interest in fixing this. Since we have been shipping std::span and we do support compilers that don't implement concepts yet, I'd like to push back against this change *for the time being*.

What I suggest is that you implement the changes discussed in https://bugs.llvm.org/show_bug.cgi?id=51443 using enable_if_t, and then we revisit this change as soon as all supported compilers implement concepts, which should be the case within a couple months. When that's the case, this patch will have all my support. Does that seem reasonable?

Seems very reasonable and what I'd recommend as well. I'll move forward with implementing http://wg21.link/p1394 using enable_if_t and as a follow-up I'll dig into the paper for the conditional explicitness of the constructors as talked about in the bug. We'll revisit this patch once we drop support for compilers that don't support concepts.

[jloser]

libcxx/include/span

237

@Quuxplusone @ldionne how do you feel about revisiting this PR now that concepts should be fine to use in <span>?

As it stands, we have a mix of enable_if_t vs concepts in some C++20 code in libc++ -- with most of the C++20-code being concepts and <span> being some of the former.

[• Quuxplusone]

libcxx/include/span

237

We're still testing buildkite configurations (on Apple targets) where libcpp-no-concepts is a thing. IMO it will be appropriate to migrate things to Concepts only after someone lands a patch removing libcpp-no-concepts from the tests and _LIBCPP_HAS_NO_CONCEPTS from the code.
(Otherwise, we'll be regressing those platforms — it'll be OK to use <span> on those platforms in 2021, but not OK to use <span> on those platforms in 2022. Also, if you do these steps out of order, the <span> step will just physically be bigger, because you'll have to manually UNSUPPORTED: libcpp-no-concepts all of its tests.)

If your hypothesis is that "by the time Clang 15 ships, these platforms will have Concepts support," then ok cool, make a PR to remove libcpp-no-concepts and see if people agree about that; then let's revisit this after that lands. :)

[jloser]

@ldionne @Mordante now that all of our supported compilers support concepts, we can revisit this patch. What's your general appetite for using concepts (here and elsewhere for C++20-or-later code)? I prefer concepts, but I think Arthur had a benchmark showing enable_if was quicker to compile IIRC.

[Mordante]

In D109212#3381221, @jloser wrote:

@ldionne @Mordante now that all of our supported compilers support concepts, we can revisit this patch. What's your general appetite for using concepts (here and elsewhere for C++20-or-later code)? I prefer concepts, but I think Arthur had a benchmark showing enable_if was quicker to compile IIRC.

Thanks for bringing this up again!

Arthur indeed had a benchmark concepts were slower. If easily done it would be nice to run this benchmark again. Compiler writers had years to optimize SFINAE, but concepts are fairly new. So I expect compiler writers to improve the speed processing concepts for the next years.

From a readability and maintainability point of view I prefer concepts.

In D109212#2982777, @ldionne wrote:

… then we revisit this change as soon as all supported compilers implement concepts, which should be the case within a couple months. When that's the case, this patch will have all my support.

Based on this comment I started reviewing, but the patch needs a rebase before reviewing. Feel free to wait until @ldionne agrees that we still want this improvement.

libcxx/include/span

467–468

These spaces look odd.

468

I was wondering why not using __is_span_compatible_container_v<_Container, _Tp>, but it seems __is_span_compatible_container no longer exists. So I stopped reviewing here. When we want to use this patch it needs to be rebased first.

[ldionne]

I would support exploring this -- unless enable_if is significantly faster, I think we should go for the readability and diagnostics improvement.

[jloser]

Rebase, apply concepts everywhere.

[jloser]

Fix spacing in template id list

[philnik]

libcxx/include/span

209

Could you rename this to __span_compatible_sentinel_for to keep the naming scheme of sized_sentinel_for and friends?

454–455

Wouldn't that also work?

479–480

Same here.

608

Is there a reason we don't just use decltype(auto) as the return type and drop the trailing return type completely?

[jloser]

Address philnik's comments

libcxx/include/span

454–455

Yes, it works, but loses the symmetry with the next function template below (constrained by __span_array_convertible<const _OtherElementType, element_type>). I originally had it as you suggested. I'll make that change and we can let others decide which they prefer; I have a slight preference towards what you suggested because it's terser.

608

No reason. Just switched to decltype(auto) since we have existing use of that in variant, and some of ranges, etc.

[jloser]

ping @philnik @Mordante @ldionne @var-const

[ldionne]

LGTM with comment.

libcxx/include/span

608

I think it's actually confusing to use decltype(auto) here since it suggests that there's some value-category preservation going on, when there really is none. I would just use auto instead.

[jloser]

Just landed this with cb48ed38b8d09ef9cfa1f7258342d0379f169074. I accidentally did it from a different branch, so this review didn't auto-close. Sorry about that. Can someone elaborate on how to manually close this review? The docs at https://llvm.org/docs/Phabricator.html seem to suggest there's a Close Revision button, but I'm not seeing it.

[Mordante]

In D109212#3584042, @jloser wrote:

Just landed this with cb48ed38b8d09ef9cfa1f7258342d0379f169074. I accidentally did it from a different branch, so this review didn't auto-close. Sorry about that. Can someone elaborate on how to manually close this review? The docs at https://llvm.org/docs/Phabricator.html seem to suggest there's a Close Revision button, but I'm not seeing it.

When you submit a comment there's an Add Action... drop down, there you can close the revision.

[Mordante]

Commandeer to close it.

[Mordante]

As mentioned the commit was landed, but from the wrong branch so it wasn't closed automatically.

libcxx/include/span

concept __span_compatible_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(*)[]>;
 #endif // !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
 
+template <class _From, class _To>
+concept __span_array_convertible = is_convertible_v<_From(*)[], _To(*)[]>;
+
+template <class _It, class _Tp>
+concept __span_compatible_iterator = contiguous_iterator<_It> && __span_array_convertible<remove_reference_t<iter_reference_t<_It>>, _Tp>;
+
+template <class _Sentinel, class _It>
+concept __span_compatible_sentinel = sized_sentinel_for<_Sentinel, _It> && !is_convertible_v<_Sentinel, size_t>;
+
 #if defined(_LIBCPP_ENABLE_DEBUG_MODE) || defined(_LIBCPP_ABI_SPAN_POINTER_ITERATORS)
 #   define _LIBCPP_SPAN_USE_POINTER_ITERATOR
 #endif
 
 template <typename _Tp, size_t _Extent>
 class _LIBCPP_TEMPLATE_VIS span {
 public:
 //  constants and types

 #else
     using iterator               = __wrap_iter<pointer>;
 #endif
     using reverse_iterator       = _VSTD::reverse_iterator<iterator>;
 
     static constexpr size_type extent = _Extent;
 
 // [span.cons], span constructors, copy, assignment, and destructor
-    template <size_t _Sz = _Extent, enable_if_t<_Sz == 0, nullptr_t> = nullptr>
+    template <size_t _Sz = _Extent> requires(_Sz == 0)
     _LIBCPP_INLINE_VISIBILITY constexpr span() noexcept : __data{nullptr} {}
 
     constexpr span           (const span&) noexcept = default;
     constexpr span& operator=(const span&) noexcept = default;
 
-    template <class _It,
-              enable_if_t<contiguous_iterator<_It> &&
-                              is_convertible_v<remove_reference_t<iter_reference_t<_It>>(*)[], element_type (*)[]>,
-                          nullptr_t> = nullptr>
+    template <__span_compatible_iterator<element_type> _It>
     _LIBCPP_INLINE_VISIBILITY
     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 <
-        class _It, class _End,
-        enable_if_t<is_convertible_v<remove_reference_t<iter_reference_t<_It> > (*)[], element_type (*)[]> &&
-                        contiguous_iterator<_It> && sized_sentinel_for<_End, _It> && !is_convertible_v<_End, size_t>,
-                    nullptr_t> = nullptr>
+    template <__span_compatible_iterator<element_type> _It, __span_compatible_sentinel<_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");
     }
 
     _LIBCPP_INLINE_VISIBILITY constexpr span(type_identity_t<element_type> (&__arr)[_Extent]) noexcept : __data{__arr} {}
 
-    template <class _OtherElementType,
-              enable_if_t<is_convertible_v<_OtherElementType(*)[], element_type (*)[]>, nullptr_t> = nullptr>
+    template <__span_array_convertible<element_type> _OtherElementType>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(array<_OtherElementType, _Extent>& __arr) noexcept : __data{__arr.data()} {}
 
-    template <class _OtherElementType,
-              enable_if_t<is_convertible_v<const _OtherElementType(*)[], element_type (*)[]>, nullptr_t> = nullptr>
+    template <class _OtherElementType>
+        requires __span_array_convertible<const _OtherElementType, element_type>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(const array<_OtherElementType, _Extent>& __arr) noexcept : __data{__arr.data()} {}
 
 #if defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
     template <class _Container>
         requires __span_compatible_range<_Container, element_type>
     _LIBCPP_INLINE_VISIBILITY
     constexpr explicit span(_Container& __c) : __data{std::data(__c)} {
       _LIBCPP_ASSERT(std::size(__c) == _Extent, "size mismatch in span's constructor (range)");
     }
     template <class _Container>
         requires __span_compatible_range<const _Container, element_type>
     _LIBCPP_INLINE_VISIBILITY
     constexpr explicit span(const _Container& __c) : __data{std::data(__c)} {
       _LIBCPP_ASSERT(std::size(__c) == _Extent, "size mismatch in span's constructor (range)");
     }
 #else
     template <__span_compatible_range<element_type> _Range>
     _LIBCPP_INLINE_VISIBILITY
     constexpr explicit span(_Range&& __r) : __data{ranges::data(__r)} {
       _LIBCPP_ASSERT(ranges::size(__r) == _Extent, "size mismatch in span's constructor (range)");
     }
 #endif
 
-    template <class _OtherElementType>
+    template <__span_array_convertible<element_type> _OtherElementType>
     _LIBCPP_INLINE_VISIBILITY
-        constexpr span(const span<_OtherElementType, _Extent>& __other,
-                       enable_if_t<
-                          is_convertible_v<_OtherElementType(*)[], element_type (*)[]>,
-                          nullptr_t> = nullptr)
+        constexpr span(const span<_OtherElementType, _Extent>& __other)
         : __data{__other.data()} {}
 
-    template <class _OtherElementType>
+    template <__span_array_convertible<element_type> _OtherElementType>
     _LIBCPP_INLINE_VISIBILITY
-        constexpr explicit span(const span<_OtherElementType, dynamic_extent>& __other,
-                       enable_if_t<
-                          is_convertible_v<_OtherElementType(*)[], element_type (*)[]>,
-                          nullptr_t> = nullptr) noexcept
+        constexpr explicit span(const span<_OtherElementType, dynamic_extent>& __other) 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
#endif
     _LIBCPP_INLINE_VISIBILITY span<const byte, _Extent * sizeof(element_type)> __as_bytes() const noexcept
     { return span<const byte, _Extent * sizeof(element_type)>{reinterpret_cast<const byte *>(data()), size_bytes()}; }
 
     _LIBCPP_INLINE_VISIBILITY span<byte, _Extent * sizeof(element_type)> __as_writable_bytes() const noexcept
     { return span<byte, _Extent * sizeof(element_type)>{reinterpret_cast<byte *>(data()), size_bytes()}; }
 
 private:
     pointer    __data;
-
 };
 
 
 template <typename _Tp>
 class _LIBCPP_TEMPLATE_VIS span<_Tp, dynamic_extent> {
-private:
-
 public:
 //  constants and types
     using element_type           = _Tp;
     using value_type             = remove_cv_t<_Tp>;
     using size_type              = size_t;
     using difference_type        = ptrdiff_t;
     using pointer                = _Tp *;
     using const_pointer          = const _Tp *;
#endif
     static constexpr size_type extent = dynamic_extent;
 
 // [span.cons], span constructors, copy, assignment, and destructor
     _LIBCPP_INLINE_VISIBILITY constexpr span() noexcept : __data{nullptr}, __size{0} {}
 
     constexpr span           (const span&) noexcept = default;
     constexpr span& operator=(const span&) noexcept = default;
 
-    template <class _It,
-              enable_if_t<contiguous_iterator<_It> &&
-                              is_convertible_v<remove_reference_t<iter_reference_t<_It> > (*)[], element_type (*)[]>,
-                          nullptr_t> = nullptr>
+    template <__span_compatible_iterator<element_type> _It>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(_It __first, size_type __count)
         : __data{_VSTD::to_address(__first)}, __size{__count} {}
 
-    template <
-        class _It, class _End,
-        enable_if_t<is_convertible_v<remove_reference_t<iter_reference_t<_It> > (*)[], element_type (*)[]> &&
-                        contiguous_iterator<_It> && sized_sentinel_for<_End, _It> && !is_convertible_v<_End, size_t>,
-                    nullptr_t> = nullptr>
+    template <__span_compatible_iterator<element_type> _It, __span_compatible_sentinel<_It> _End>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(_It __first, _End __last)
         : __data(_VSTD::to_address(__first)), __size(__last - __first) {}
 
     template <size_t _Sz>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(type_identity_t<element_type> (&__arr)[_Sz]) noexcept : __data{__arr}, __size{_Sz} {}
 
-    template <class _OtherElementType, size_t _Sz,
-              enable_if_t<is_convertible_v<_OtherElementType(*)[], element_type (*)[]>, nullptr_t> = nullptr>
+    template <class _OtherElementType, size_t _Sz>
+        requires __span_array_convertible<_OtherElementType, element_type>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(array<_OtherElementType, _Sz>& __arr) noexcept : __data{__arr.data()}, __size{_Sz} {}
 
-    template <class _OtherElementType, size_t _Sz,
-              enable_if_t<is_convertible_v<const _OtherElementType(*)[], element_type (*)[]>, nullptr_t> = nullptr>
+    template <class _OtherElementType, size_t _Sz>
+        requires __span_array_convertible<const _OtherElementType, element_type>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(const array<_OtherElementType, _Sz>& __arr) noexcept : __data{__arr.data()}, __size{_Sz} {}
 
 #if defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
     template <class _Container>
         requires __span_compatible_range<_Container, element_type>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(_Container& __c) : __data(std::data(__c)), __size{std::size(__c)} {}
     template <class _Container>
         requires __span_compatible_range<const _Container, element_type>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(const _Container& __c) : __data(std::data(__c)), __size{std::size(__c)} {}
 #else
     template <__span_compatible_range<element_type> _Range>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span(_Range&& __r) : __data(ranges::data(__r)), __size{ranges::size(__r)} {}
 #endif
 
     template <class _OtherElementType, size_t _OtherExtent>
+        requires __span_array_convertible<_OtherElementType, element_type>
     _LIBCPP_INLINE_VISIBILITY
-        constexpr span(const span<_OtherElementType, _OtherExtent>& __other,
-                       enable_if_t<
-                          is_convertible_v<_OtherElementType(*)[], element_type (*)[]>,
-                          nullptr_t> = nullptr) noexcept
+        constexpr span(const span<_OtherElementType, _OtherExtent>& __other)  noexcept
         : __data{__other.data()}, __size{__other.size()} {}
 
 //    ~span() noexcept = default;
 
     template <size_t _Count>
     _LIBCPP_INLINE_VISIBILITY
     constexpr span<element_type, _Count> first() const noexcept
     {

 
 //  as_bytes & as_writable_bytes
 template <class _Tp, size_t _Extent>
 _LIBCPP_INLINE_VISIBILITY
 auto as_bytes(span<_Tp, _Extent> __s) noexcept
 -> decltype(__s.__as_bytes())
 { return    __s.__as_bytes(); }
 
-template <class _Tp, size_t _Extent>
+template <class _Tp, size_t _Extent> requires(!is_const_v<_Tp>)
 _LIBCPP_INLINE_VISIBILITY
 auto as_writable_bytes(span<_Tp, _Extent> __s) noexcept
--> enable_if_t<!is_const_v<_Tp>, decltype(__s.__as_writable_bytes())>
+-> decltype(__s.__as_writable_bytes())
 { return __s.__as_writable_bytes(); }
 
 #if _LIBCPP_STD_VER > 17
 template<contiguous_iterator _It, class _EndOrSize>
     span(_It, _EndOrSize) -> span<remove_reference_t<iter_reference_t<_It>>>;
 #endif // _LIBCPP_STD_VER > 17
 
 template<class _Tp, size_t _Sz>