[libcxx-commits] [PATCH] D112510: [libc++] P0433R2: add the remaining deduction guides.

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

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

Thanks for working on this!



================
Comment at: libcxx/docs/Status/Cxx17.rst:43
 
-   .. [#note-P0433] P0433: So far, only the ``<string>``, sequence containers, container adaptors and ``<regex>`` portions of P0433 have been implemented.
+   .. [#note-P0433] P0433: The only part not fully implemented is the requirement that certain deduction guides should not participate in overload resolution when given incorrect template arguments.
    .. [#note-P0607] P0607: The parts of P0607 that are not done are the ``<regex>`` bits.
----------------
I assume you've basically done the work already to figure out //which// guides you're talking about; could you specify that exact list right here? (Or even better, make a second PR that just fixes them ;))


================
Comment at: libcxx/include/valarray:1086
 
+template<class _Tp, size_t _Size> valarray(const _Tp(&)[_Size], size_t) -> valarray<_Tp>;
+
----------------
Please line-break between `>` and `valarray`.


================
Comment at: libcxx/test/std/numerics/numarray/template.valarray/valarray.cons/deduct.pass.cpp:21-28
+    // Deducing from an array of primitives.
+    {
+        typedef int T;
+        T a[] = {1, 2, 3, 4, 5};
+        const unsigned N = sizeof(a)/sizeof(a[0]);
+        std::valarray v(a, N);
+        ASSERT_SAME_TYPE(decltype(v)::value_type, T);
----------------
Instead of these three tests, I'd rather see these seven tests (to hit all the expected CTAD codepaths):
```
{
    std::valarray v = {1, 2, 3, 4, 5};  // from initializer_list<T>
    ASSERT_SAME_TYPE(decltype(v), std::valarray<int>);
}
{
    long a[] = {1, 2, 3, 4, 5};
    std::valarray v(a, 5);  // from const T(&)[N], size_t
    ASSERT_SAME_TYPE(decltype(v), std::valarray<long>);
}
{
    long a[] = {1, 2, 3, 4, 5};
    std::valarray v(&a[0], 5);  // from const T&, size_t
    ASSERT_SAME_TYPE(decltype(v), std::valarray<long*>);  // surprising but true
}
{
    std::valarray<long> v {1,2,3,4,5};
    std::valarray v2 = v[std::slice(2,5,1)];  // from slice_array<T>
    static_assert(std::is_same_v<decltype(v2), std::valarray<long>>);
}
{
    std::valarray<long> v {1,2,3,4,5};
    std::valarray v2 = v[std::gslice(0, {5}, {1})];  // from gslice_array<T>
    static_assert(std::is_same_v<decltype(v2), std::valarray<long>>);
}
{
    std::valarray<long> v = {1, 2, 3, 4, 5};
    std::valarray<bool> m = {true, false, true, false, true};
    std::valarray v2 = v[m];  // from mask_array<T>
    static_assert(std::is_same_v<decltype(v2), std::valarray<long>>);
}
{
    std::valarray<long> v = {1, 2, 3, 4, 5};
    std::valarray<size_t> i = {1, 2, 3};
    std::valarray v2 = v[i];  // from indirect_array<T>
    static_assert(std::is_same_v<decltype(v2), std::valarray<long>>);
}
```


================
Comment at: libcxx/test/std/utilities/allocator.adaptor/allocator.adaptor.cnstr/deduct.pass.cpp:29
+        std::scoped_allocator_adaptor a(outer);
+        ASSERT_SAME_TYPE(decltype(a)::outer_allocator_type, OuterAlloc);
+    }
----------------
Here and throughout, please check the actual type of `a`:
```
ASSERT_SAME_TYPE(decltype(a), std::scoped_allocator_adaptor<OuterAlloc>);
```
Checking the nested `::outer_allocator_type` typedef isn't quite as good; and although checking //both// would be harmless, it also wouldn't be helpful. All we really care about in this specific test file is that CTAD produces the exactly correct `scoped_allocator_adaptor<...>` type; the existence/values of these nested typedefs can (and should already be) tested elsewhere.


================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/deduct.pass.cpp:40-65
+    {
+//  optional(const T&);
+    const int& source = 5;
+    std::optional opt(source);
+    ASSERT_SAME_TYPE(decltype(opt), std::optional<int>);
+    assert(static_cast<bool>(opt));
+    assert(*opt == 5);
----------------
FWIW, these three new tests strike me as redundant with line 35 (which itself — pre-existing — is already redundant with line 27).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112510