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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM!



================
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.
----------------
var-const wrote:
> 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.
If there's a chance someone else will have to do the work later, then it's important to have a really good and unambiguous TODO list here. But if you're just holding the list in your head while //you// actively do the work, then this is fine.
It sounds like the latter is the case; therefore I'm OK with this.


================
Comment at: libcxx/test/std/numerics/numarray/template.valarray/valarray.cons/deduct.pass.cpp:45
+        std::valarray<long> v{1,2,3,4,5};
+        std::valarray v2 = v[std::slice(2,5,1)];
+        static_assert(std::is_same_v<decltype(v2), std::valarray<long>>);
----------------
ASAN rightly complains because I forgot that the second argument is "size", not "end". Make this `std::slice(2,3,1)` and I think it'll be quiet.


================
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);
----------------
var-const wrote:
> 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.
Personally I do still think they're unneeded; but Louis' preference overrules mine. Keep.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112510