[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 a comment.

@mclow.lists: I think everything's resolved and this is ready for you (or someone; not me) to land. Once it's landed, I assume the next step will be to resume review on D58582 <https://reviews.llvm.org/D58582> (set) and/or D58590 <https://reviews.llvm.org/D58590> (unordered_map).



================
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:
> > > 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.
> > I think you think we're discussing different things, but we're not. ;)
> > Here's the test in question:
> > 
> >     std::map<int, long> source;
> >     std::map m(source, std::allocator<int>());
> >     ASSERT_SAME_TYPE(decltype(m), decltype(source));
> > 
> > I claim (equivalently, P1518 claims) that the variable definition on line 2 creates a variable `m` of type `std::map<int, long>`. This is "obvious" because it is calling the allocator-aware constructor `map(const map&, allocator)`. We're using CTAD here, but we could just as well put the deduced template arguments back in place:
> > 
> >     std::map<int, long> m(source, std::allocator<int>());
> > 
> > This is fine. The second constructor argument, the one that provides a "value" for the new map's allocator, is implicitly converted from `std::allocator<int>` to `std::map<int, long>::allocator_type`.
> > 
> > You might say "Shouldn't the second parameter's type be forced to //exactly match// `std::map<int, long>::allocator_type`?" but no, that's not how it works without CTAD, so it's not how it should work with CTAD either. In particular, we would like the following to work:
> > 
> >     std::pmr::map<int, long> source;
> >     std::map m(source, std::pmr::new_delete_resource());
> >     ASSERT_SAME_TYPE(decltype(m), decltype(source));
> > 
> >         // C++2a only: requires CTAD to see through typedefs
> >     std::pmr::map m2(source, std::pmr::new_delete_resource());
> >     ASSERT_SAME_TYPE(decltype(m2), decltype(source));
> > 
> > That can only work (AFAIK) if you don't allow CTAD to "get confused" by the actual argument type of that second argument. All we require is that the actual argument should be something convertible to `allocator_type` (which would be the requirement in the absence of CTAD).
> > 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 that we should remove this test, commit the rest of this patch, and then continue this debate  if/when P1518 lands.  Tests that are commented out are always a question for the reader: "Why is this test commented out?"
> 
Okay. Not removed, but modified to use the exact `allocator_type`, which we both agree should work regardless.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D58587