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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
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>
----------------
Quuxplusone wrote:
> 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));
> }
> ```
FWIW, the Standard does not request that we check for anything more than "an integral type is not an input iterator". I agree we should change the tests that currently pass a `struct BadIter { };` to make them pass a `using BadIter = some-output-iterator;`. And we need to mark those as libc++ specific, since that's technically just our QOI.


================
Comment at: libcxx/test/support/deduction_guides_sfinae_checks.h:57
+template<template<typename ...> class Container, typename ValueType>
+constexpr void AssociativeContainerDeductionGuidesSfinaeAway() {
+  using Comp = std::less<int>;
----------------
Per our offline discussion, let's add a test where we pass an integer type as an iterator and watch it fail, and also a libc++ specific test that passes an output iterator.

Note that we can't add an integer type test to the sequence containers, because those won't SFINAE away (the sequence containers can deduce from `FOO(size_type, value_type)` when passed two integers).


================
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);
----------------
Quuxplusone wrote:
> 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`).
I disagree with that. I looked at the PR in a previous stage where the checks were duplicated, and there was *a lot* of code duplication. Since most of these requirements are specified in http://eel.is/c++draft/container.requirements.general, I think it makes sense to colocate the tests the way they are right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112904