[libcxx-commits] [PATCH] D114311: [libcxx] Make sequential containers slightly more SFINAE-friendly during CTAD.

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

In D114311#3144651 <https://reviews.llvm.org/D114311#3144651>, @Quuxplusone wrote:

> For testing, I think you've got the right idea: the only way to test this (besides ugly/fragile .verify.cpp tests) will be SFINAE. You just implemented `SequenceContainerDeductionGuidesSfinaeAway` in a previous patch, right? So shouldn't this just be a one- or two-line addition to that function?
>
>   // Cannot deduce from (size_type, value_type, BAD_alloc)
>   static_assert(SFINAEs_away<Container, size_t, void*, BadAlloc>);
>
> If that doesn't work as a regression-test, then I'm confused about the effect of this patch.

Yes, this works (the existing

  // Cannot deduce from (iter, iter, BAD_alloc)
  static_assert(SFINAEs_away<Container, Iter, Iter, BadAlloc>);

works as well -- it would give a hard error without either this patch or the previous approach that SFINAEd away `allocator_traits`.

What I meant to say is that it's hard to test for the difference between having one or two arguments in the template function:

  // This is, IIUC, the correct form because it makes sure that `__is_allocator` is called on a deduced type.
  template <class _MaybeAllocator = _Allocator, class = __enable_if_t<__is_allocator<_MaybeAllocator>::value> >
  // However, this works as well, even though `_Allocator` is not, AFAICT, a deduced type. It seems to me
  // that it shouldn't work and perhaps Clang is being permissive here -- otherwise I must be missing something.
  template <class = __enable_if_t<__is_allocator<_Allocator>::value> >

I couldn't think of a great way to test this difference. In existing tests, it only shows up in certain type traits, e.g. `static_assert(std::is_default_constructible_v<T>)` works with the 2-argument form but not 1-argument form. However, it seemed like something that _happened_ to depend on this difference rather than actively check for it, if that makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114311