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

Mon Oct 11 13:58:06 PDT 2021

jloser added inline comments.

================
Comment at: libcxx/include/span:221
 // [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} {}
----------------
jloser wrote:
> Quuxplusone wrote:
> > > `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?
> 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?).
@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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109212