[libcxx-commits] [PATCH] D109212: [libc++][NFCI] span: replace enable_if with requires

[Mark de Wever via Phabricator via libcxx-commits]

Mordante requested changes to this revision.
Mordante added a comment.

In D109212#3381221 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.



================
Comment at: libcxx/include/span:418
     _LIBCPP_INLINE_VISIBILITY
-        constexpr span(      _Container& __c,
-            enable_if_t<__is_span_compatible_container<_Container, _Tp>::value, nullptr_t> = nullptr)
+        constexpr span(      _Container& __c)
+            requires(__is_span_compatible_container<_Container, _Tp>::value)
----------------
These spaces look odd.


================
Comment at: libcxx/include/span:419
+        constexpr span(      _Container& __c)
+            requires(__is_span_compatible_container<_Container, _Tp>::value)
         : __data{_VSTD::data(__c)}, __size{(size_type) _VSTD::size(__c)} {}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109212