[libcxx-commits] [PATCH] D58587: Implement P0433: deduction guides for <map>

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

Quuxplusone marked 3 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: test/std/containers/associative/map/map.cons/deduct.pass.cpp:87
+    std::map<int, long> source;
+    std::map m(source, std::allocator<int>());
+    ASSERT_SAME_TYPE(decltype(m), decltype(source));
----------------
mclow.lists wrote:
> Quuxplusone wrote:
> > mclow.lists wrote:
> > > This works? I would think that it should not. We rebind the allocator internally into something else, but we should probably reject this. [But not in this patch]
> > This is me getting a jump start on [P1518 Stop overconstraining allocators in container deduction guides](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1518r0.html#overconst2).
> > 
> > Since
> > 
> >     std::map<int, long> m(source, std::allocator<int>());
> > 
> > works (without CTAD), Mike Spertus (as of February) and I believe that
> > 
> >     std::map m(source, std::allocator<int>());
> > 
> > should work //with// CTAD as well.
> I don't believe that either should work.
> 
> `std::vector<long, std::allocator<int>> m;`
> doesn't work, nor does:
> ```
> long l[] = {1L };
> std::vector v(std::begin(l), std::end(l), std::allocator<int>());
> ```
> 
> The fact that it works today for `map` and `set` is due to insufficient checking in libc++
`std::vector<long, std::allocator<int>> m;` is not expected to work, because it avoids CTAD and explicitly instantiates `vector` with bad template parameters.

`std::vector v(std::begin(l), std::end(l), std::allocator<int>())` is not expected to work either (although there's a slightly stronger case for allowing it to work), because the value type of the iterator is different from the value type of the-allocator-which-is-//deduced//-by-CTAD.  (Since the allocator is deduced, there's arguably a case for CTAD to trust the allocator's value_type and ignore the irrelevant value type of the iterator. That is, arguably, CTAD could do the same thing that `std::vector<int, std::allocator<int>> v(std::begin(l), std::end(l), std::allocator<int>())` does.)

But in `std::vector w(source, std::allocator<int>())`, CTAD knows we're using the allocator-aware copy constructor of `vector`. That constructor deduces the allocator type from the //first// parameter — the vector it's making a copy of — not from the second parameter. The only requirement is that the passed-in allocator argument should be //implicitly convertible to// the deduced allocator type. Which in this case, it is.

I'm willing to `#if 0` out this test for now, but I am also willing to keep debating it until you see the P1518 light. ;)


================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct_const.pass.cpp:123
+    assert(m.get_allocator().get_id() == 45);
+    }
+
----------------
mclow.lists wrote:
> mclow.lists wrote:
> > Quuxplusone wrote:
> > > BTW, if you told me to remove these last two tests (which test `std::multimap<const int&, long>`), I would do so.
> > > I don't know whether libc++ is supporting reference types here by unhappy accident (i.e. you should just add `static_assert(is_object_v<_Key> && is_object_v<_Tp>)` and be done with it), or by happy accident (i.e. you would be scared to add that `static_assert` at this point but I should remove these two tests anyway), or on purpose (i.e. I should keep these two tests).
> > > 
> > > Note that `std::map` cannot support a reference-typed `_Key` because then `try_emplace` would give multiple declaration errors: `const _Key&` and `_Key&&` would be the same type.
> > I'll look into this.
> I would say drop these two tests. We don't have any other map tests that use reference keys.
Will do.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D58587