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

Marshall Clow via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Fri May 3 09:19:04 PDT 2019


mclow.lists added a comment.

Here's a bunch of minor stylistic and comment nits.

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



================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct.fail.cpp:60
+    {
+        // refuse to rebind the allocator if Allocator::value_type is not exactly what we expect
+        // std::pair<int, int> arr[] = { {1,1}, {2,2}, {3,3} };
----------------
What's going on here?
Does this test need to be uncommented? or removed?


================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct.fail.cpp:65
+    {
+        NotAnAllocator a;
+        std::multimap m(a); // expected-error{{no viable constructor or deduction guide for deduction of template arguments of 'multimap'}}
----------------
Missing motivation for this bit.


================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct.pass.cpp:39
+int main(int, char**)
+{
+    {
----------------
All though this test, you have a mixture of `pair<int, long>` and `pair<const int, long>`. Are these changes relevant?  If you need them for the `equals` calls, you can define a custom comparison:

```
template <class T1, class T2>
struct pair_eq
{
	bool operator()(const std::pair<      T1,       T2> &rhs, const std::pair<      T1,       T2> &lhs) const
	{ return lhs.first == rhs.first && lhs.second == rhs.second; }

	bool operator()(const std::pair<const T1, const T2> &rhs, const std::pair<const T1, const T2> &lhs) const
	{ return lhs.first == rhs.first && lhs.second == rhs.second; }

	bool operator()(const std::pair<      T1, const T2> &rhs, const std::pair<const T1, const T2> &lhs) const
	{ return lhs.first == rhs.first && lhs.second == rhs.second; }

	bool operator()(const std::pair<const T1, const T2> &rhs, const std::pair<      T1, const T2> &lhs) const
	{ return lhs.first == rhs.first && lhs.second == rhs.second; }
};
```

and then call 
`assert(std::equal(m.begin(), m.end(), std::begin(expected_m), std::end(expected_m, pair_eq<int, long>{})));`

I'm not 100% sure that this is a win, but it might be.


================
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} };
----------------
Alternately, you could define a couple of typedefs:

    using P = std::pair<int, long>;
    using PC = std::pair<const int, long>;



================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct.pass.cpp:45
+    ASSERT_SAME_TYPE(decltype(m), std::multimap<int, long>);
+    std::pair<const int, long> expected_m[] = { {1,1L}, {1,1L}, {2,2L}, {3,1L}, {INT_MAX,1L} };
+    assert(std::equal(m.begin(), m.end(), std::begin(expected_m), std::end(expected_m)));
----------------
This should be const, too. (I know that you don't ever change this, but ...)


================
Comment at: test/std/containers/associative/multimap/multimap.cons/deduct.pass.cpp:77
+    {
+    std::multimap<int, long> source;
+    std::multimap m{source};
----------------
A comment here "braces instead of parens" would help a lot.


================
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} };
+
----------------
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} };`


================
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)));
----------------
Why is this not `P expected_m[] = ...`?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D58587





More information about the libcxx-commits mailing list