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

[Marshall Clow via Phabricator via libcxx-commits]

mclow.lists 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));
----------------
Quuxplusone wrote:
> 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. ;)
I think we're discussing different things here. 

 I'm talking about the general container requirements, the very first line in Table 62, which says:
> Mandates: `allocator_­type::value_­type` is the same as `X::value_­type`.

As far as I can tell, P1518 doesn't change that at all.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D58587