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

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

Quuxplusone marked 5 inline comments as done.
Quuxplusone added a comment.

> I'm irked by the `typedef typename __identity<_Allocator>::type allocator_type;` in the definition of map/multimap. What problem is that solving?

Unfortunately, if you don't place a "deduction firewall" between `allocator_type` and `_Allocator`, then the deduction guides implicitly generated from the unconstrained constructors of map/multimap that take an `allocator_type` parameter will happily match and deduce e.g. `_Allocator=std::less<int>`, which then blows up with a hard error trying to instantiate `allocator_traits<std::less<int>>`. We need to place that firewall in order to nerf the implicitly generated deduction guides, so that our explicit deduction guides can take priority.

Easy experiment: remove the firewall and compile the new test files. If I'm not mistaken, you'll see horrible error messages.



================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct.pass.cpp:40
+{
+    {
+    const std::pair<int, long> arr[] = { {1,1L}, {2,2L}, {1,1L}, {INT_MAX,1L}, {3,1L} };
----------------
mclow.lists wrote:
> Alternately, you could define a couple of typedefs:
> 
>     using P = std::pair<int, long>;
>     using PC = std::pair<const int, long>;
> 
This sounds good.
Yes, I believe I needed to use exactly `value_type` so that the `equals` would work.
Will change to use those two typedefs and `const PC expected_m[] = ...`.


================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct.pass.cpp:91
+    {
+    std::multimap m{ std::pair{1,1L}, std::pair{2,2L}, std::pair{1,1L}, std::pair{INT_MAX,1L}, std::pair{3,1L} };
+
----------------
mclow.lists wrote:
> typedef `using P = std::pair<int, long>;` would make this clearer.
> Along with ` std::multimap m{ P{1,1L}, P{2,2L}, P{1,1L}, P{INT_MAX,1L}, P{3,1L} };`
Hmm. I seem to recall that I specifically didn't want to nail down what kind of `pair` was being CTAD'ed — the idea was to keep this specific code (a CTAD `multimap` of `pair{x,y}`s) working even if the CTAD guides on either `multimap` or `pair` are changed in the future. Other tests would handle "a CTAD `multimap` of `pair<A,B>(x,y)`s" and "a CTAD `multimap` of `{x,y}`s."
Except that I don't see those tests here! So I guess using the concrete type `pair<A,B>` (or just `P`) would be okay.

I am unsure whether `multimap{ pair{a,b}, pair{c,d} }` can ever produce a different type from `multimap{ pair<A,B>{a,b}, pair<A,B>{c,d} }`. I am pretty sure that those can produce a different type from `multimap{ {a,b}, {c,d} }`.


================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct.pass.cpp:111
+    ASSERT_SAME_TYPE(decltype(m), std::multimap<int, long, std::greater<int>, test_allocator<P>>);
+    std::pair<const int, long> expected_m[] = { {INT_MAX,1L}, {3,1L}, {2,2L}, {1,1L}, {1,1L} };
+    assert(std::equal(m.begin(), m.end(), std::begin(expected_m), std::end(expected_m)));
----------------
mclow.lists wrote:
> Why is this not `P expected_m[] = ...`?
Will change to `const PC expected_m[] = ...`.
(I think originally I introduced typedef `P` //only// for use in `allocator<P>`, because those lines were getting too long. So it wasn't used consistently everywhere — just in the allocator types.)


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D58587