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

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Mon Oct 11 14:05:30 PDT 2021


Quuxplusone 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:
> 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.
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. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109212



More information about the libcxx-commits mailing list