[libcxx-commits] [PATCH] D61771: Comparing Unordered Containers (P0809)

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 5 inline comments as done.
zoecarver added inline comments.


================
Comment at: include/unordered_map:1641
+#if _LIBCPP_STD_VER > 17
+template <class _Key, class _Tp, class _Hash1, class _Pred, class _Alloc1,
+                                 class _Hash2,              class _Alloc2>
----------------
mclow.lists wrote:
> zoecarver wrote:
> > Was it ever necessary for the two maps to use the same allocator type (and is it now)? Furthermore, is it even necessary for them to have the same key/value type (I don't see anything in the standard that explicitly says they need to be)? 
> All the container comparisons in the standard are defined in terms of comparing two containers of the same type. For allocator-aware containers, this includes the allocators.  There is a proposal to relax that in flight (https://wg21.link/P0805), but I don't think that will be part of C++20.
> 
> My suggestion is to not support heterogenous comparisons at this time.
> Note that this includes the hasher as well as the predicate and the allocator.
I will keep an eye on that paper. As per your below comment, I will get rid of this. 


================
Comment at: include/unordered_map:1656
+        _EqRng __yeq = __y.equal_range(__i->first);
+        if (_VSTD::distance(__xeq.first, __xeq.second) !=
+                _VSTD::distance(__yeq.first, __yeq.second) ||
----------------
mclow.lists wrote:
> This can be simplified for `unordered_map`, because `unordered_map` requires unique keys.
> 
> You can just use `__y.find` instead of `equal_range` and a single (Still need `equal_range` etc for `unordered_multimap`, though)
> 
Good idea! I will change this.


================
Comment at: include/unordered_set:968
+    {
+        _EqRng __xeq = __x.equal_range(*__i);
+        _EqRng __yeq = __y.equal_range(*__i);
----------------
mclow.lists wrote:
> Like the `unordered_map` code, this can be made much simpler because there are unique keys.
Good catch. Will update (same with plain unordered_set).


================
Comment at: include/unordered_set:1006
+                _VSTD::distance(__yeq.first, __yeq.second) ||
+            !_VSTD::is_permutation(__xeq.first, __xeq.second, __yeq.first))
             return false;
----------------
mclow.lists wrote:
> No need for `is_permutation`, again.
The standard specifically says to use `is_permutation` (for both set and multiset, etc.). 


================
Comment at: test/std/containers/unord/unord.map/comp_different_hash.pass.cpp:34
+
+template<class Map, class Itter>
+void fill(Map& map, Itter begin, Itter end)
----------------
mclow.lists wrote:
> I don't think that this is what P0809 meant.  "Hashes with different behavior", yes.
> "Hashes of different types", no.
> 
While the paper doesn't explain it specifically, I think you are right. Because of the part that is removed which defines the //behavior// and not //type//. I can update it :P 


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D61771