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

Fri Sep 3 11:05:08 PDT 2021

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109212