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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/set:504
     typedef key_type                                 value_type;
-    typedef _Compare                                 key_compare;
+    typedef __identity_t<_Compare>                   key_compare;
     typedef key_compare                              value_compare;
----------------
var-const wrote:
> Without this change (which is used in `map` and `multimap`) certain incorrect invocations try to do instantiations that depend on `_Compare`, leading to verbose errors.
I vaguely recall hitting similar issues around CTAD in these containers, and suggesting that we firewall several (maybe all) of these member typedefs with `__identity_t`. But back then there was some reason we didn't do that. Probably just inertia, but I'm not sure. Looks like some discussion exists in D58587.


================
Comment at: libcxx/include/vector:492
+    typedef typename __base::const_reference                         const_reference;
+    typedef typename __allocator_traits<allocator_type>::size_type   size_type;
+    typedef typename __base::difference_type                         difference_type;
----------------
I still don't get this diff; but is it possible that it'll be a moot point after D112976?


================
Comment at: libcxx/test/support/deduction_guides_sfinae_checks.h:23-32
+template<template<typename ...> class Instantiated, class ...CtrArgs,
+    class = decltype(Instantiated(std::declval<CtrArgs>()...))>
+std::false_type SFINAEs_away_impl(int);
+
+template<template<typename ...> class Instantiated, class ...CtrArgs>
+std::true_type SFINAEs_away_impl(...);
+
----------------
Even after seeing @ldionne's comment, personally, I'd still write these tests without any additional headers. For a sequence container, we'd have this:
```
template<class... Args>
auto deque_has_no_ctad_from() -> decltype(std::deque(std::declval<Args>()...));
template<class... Args>
constexpr bool deque_has_no_ctad_from() { return true; }

int main(int, char**)
{
    // all the deduct.pass.cpp tests, and then these lines:

    using OutIt = std::insert_iterator<std::vector<int>>;
    struct NotAnAllocator {};

    LIBCPP_STATIC_ASSERT(deque_has_no_ctad_from<OutIt, OutIt>());
    LIBCPP_STATIC_ASSERT(deque_has_no_ctad_from<OutIt, OutIt, std::allocator<int>>());
    static_assert(deque_has_no_ctad_from<int*, int*, NotAnAllocator>());
}
```
And for an associative container like `map` we'd have this:
```
template<class... Args>
auto map_has_no_ctad_from() -> decltype(std::map(std::declval<Args>()...));
template<class... Args>
constexpr bool map_has_no_ctad_from() { return true; }

int main(int, char**)
{
    // all the deduct.pass.cpp tests, and then these lines:

    using VT = std::pair<const int, int>;
    using OutIt = std::insert_iterator<std::vector<int>>;
    struct NotAnAllocator {};

    static_assert(map_has_no_ctad_from<int, int>());
    static_assert(map_has_no_ctad_from<int, int, std::less<int>>());
    static_assert(map_has_no_ctad_from<int, int, std::allocator<VT>>());
    static_assert(map_has_no_ctad_from<int, int, std::less<int>, std::allocator<VT>>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::less<int>>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::allocator<VT>>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<OutIt, OutIt, std::less<int>, std::allocator<VT>>());
    static_assert(map_has_no_ctad_from<VT*, VT*, std::allocator<VT>, std::allocator<VT>>());
    static_assert(map_has_no_ctad_from<VT*, VT*, std::less<int>, NotAnAllocator>());
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<std::initializer_list<VT>, std::allocator<VT>, std::allocator<VT>>);
    LIBCPP_STATIC_ASSERT(map_has_no_ctad_from<std::initializer_list<VT>, std::less<int>, NotAnAllocator>);
}
```
I just don't see the need for 300 lines of code off in test/support/ (plus another 100 lines in the priority_queue test) when we could have ~20 lines per container, right there in each container's test directory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112904