[libcxx-commits] [PATCH] D112904: [libc++] P0433R2: test that deduction guides are properly SFINAEd away.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.

I've looked only at the new `deque` code, but my comments probably apply equally to all the containers.



================
Comment at: libcxx/include/deque:1269
+    typedef typename __base::const_iterator                          const_iterator;
+    typedef typename __allocator_traits<allocator_type>::size_type   size_type;
+    typedef typename __base::difference_type                         difference_type;
----------------
I don't believe this is needed. By the time we are instantiating the members of `deque<T, allocator_type>`, we are guaranteed that `allocator_type` is an Allocator type.


================
Comment at: libcxx/include/deque:1572
          class _Alloc = allocator<__iter_value_type<_InputIterator>>,
+         class = enable_if_t<__is_cpp17_input_iterator<_InputIterator>::value>,
          class = enable_if_t<__is_allocator<_Alloc>::value>
----------------
FWIW, `__iter_value_type<_InputIterator>` means `typename iterator_traits<_InputIterator>::value_type`, which is ill-formed (SFINAE-friendly) unless `_InputIterator` is an iterator type. However, I guess what you're doing here is protecting against the possibility that `_InputIterator` could be an //output// iterator? ...But you have added no tests for that new behavior, AFAICT.

Please replace all your new tests with something simple that demonstrates the new behavior, e.g. https://godbolt.org/z/aKeh1fE9Y
```
template<class... Args>
auto deque_has_ctad_from(int, Args&&... args) -> decltype(std::deque(std::forward<Args>(args)...), true) { return true; }
template<class... Args>
bool deque_has_ctad_from(long, Args&&...) { return false; }

int main() {
    std::vector<int> v;
    auto out = std::back_inserter(v);
    assert(!deque_has_ctad_from(42, out, out));
}
```


================
Comment at: libcxx/test/support/sfinaes_away.h:15-28
+// `SFINAEs_away` template variable checks whether the template arguments for
+// a given template class `Instantiated` can be deduced from the given
+// constructor parameter types `CtrArgs` using CTAD.
+
+template<template<typename ...> class Instantiated, class ...CtrArgs,
+    class = decltype(Instantiated(std::declval<CtrArgs>()...))>
+std::false_type SFINAEs_away_impl(int);
----------------
ldionne wrote:
> IMO this utility is a bit too narrow in usefulness to make it its own header, since it only works for checking that a class template's CTAD SFINAEs away. If this were for checking that a general expression SFINAEs away, then I'd be in favour of keeping it standalone.
> 
> I would suggest either moving it to `deduction_guides_sfinae_checks.h` or generalizing it.
+1 and more so: I don't think this PR should add //any// new headers. Each "deduct.pass.cpp" should just introduce a four-line `bool foo_has_ctad_from(42, args...)` (for the appropriate container-name `foo`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112904