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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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.
----------------
ldionne wrote:
> Quuxplusone wrote:
> > 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 ;))
> I think @var-const 's intent was to come back and fix them in a future PR. The guides he's talking about here are things like SFINAEing `unordered_map` guides away when the provided iterator is not an input iterator. We have a couple such guides that are not 100% correctly implemented.
There is a requirement for all containers that deduction guides should not participate in overload resolution when the given types do not qualify as input iterators / allocators / comparators / hash functors. This requirement is largely untested and I think in some cases not fully implemented. There is also a requirement that a `unique_ptr` cannot be deduced from a plain pointer (to prevent a case where deducing from a built-in array would also deduce a pointer) -- it's implemented correctly but not tested.

I thought that listing all of this would be a little too much detail, and I'm indeed working on addressing those in a follow-up, so I thought it's okay to be a little more vague in the description. Please let me know if you'd still like to expand the description.


================
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);
----------------
Quuxplusone wrote:
> 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>>);
> }
> ```
Thanks a lot! Added.


================
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);
----------------
ldionne wrote:
> Quuxplusone wrote:
> > FWIW, these three new tests strike me as redundant with line 35 (which itself — pre-existing — is already redundant with line 27).
> IMO those are all relevant. The `const T&` one makes sure that we handle references correctly, the `T*` one that we handle pointers correctly, and the `T[]` one that arrays will cause the deduction guide to deduce an optional pointer.
> 
> Sure, when you know how it's implemented you realize that it's obvious it's going to work, but if you don't know how the deduction guide is implemented, IMO these test cases are useful.
@Quuxplusone Arthur, please let me know if you feel strongly about removing these. I'm slightly inclined to keep these tests (my intention was pretty much what Louis described), but I'm ok to remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112510