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

Fri Sep 3 12:40:35 PDT 2021

jloser added a comment.

In D109212#2982777 <https://reviews.llvm.org/D109212#2982777>, @ldionne wrote:

> In D109212#2982390 <https://reviews.llvm.org/D109212#2982390>, @jloser wrote:
>
>> In D109212#2981554 <https://reviews.llvm.org/D109212#2981554>, @Mordante wrote:
>>
>>> Thanks for working on this! In general I prefer concepts over `enable_if`s. Unfortunately some of our supported compilers don't have proper concepts support yet. We could use `#if`s 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 <https://bugs.llvm.org/show_bug.cgi?id=51443>. 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109212