[libcxx-commits] [PATCH] D87171: Implement P0919R3

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added subscribers: zoecarver, curdeius.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks for the patch! This is looking pretty good, with some minor comments.

Misc comment: I think we're missing the `__cpp_lib_generic_unordered_lookup` feature test macro?

Perhaps my mind is playing tricks to me, but I have the feeling someone else had been working on this before. @zoecarver  @curdeius perhaps? In all cases, please do update this revision as we will take it. If someone else had been working on this, we could steal some tests from that other revision if relevant.



================
Comment at: libcxx/include/unordered_map:473
+#if _LIBCPP_STD_VER > 17
+    template <typename _K2>
+    _LIBCPP_INLINE_VISIBILITY
----------------
Can you use this pattern instead?

```
template <typename _K2, typename = enable_if_t<__is_transparent<_Hash, _K2>::value && __is_transparent<_Pred, _K2>::value>>
size_t ...
```

It usually leads to easier-to-read code since the return type isn't hidden at the end of `enable_if_t`.



================
Comment at: libcxx/include/unordered_map:1357
+    #if _LIBCPP_STD_VER > 17
+        template <typename _K2> requires __is_transparent<hasher, _K2>::value && __is_transparent<key_equal, _K2>::value
+        _LIBCPP_INLINE_VISIBILITY
----------------
rarutyun wrote:
> I may do it without requires for all the places with just using the common approach with enable_if since not all compilers support concepts/constraints even if _LIBCPP_STD_VER > 17
Yes, please use `enable_if` instead of `requires`.


================
Comment at: libcxx/test/std/containers/unord/unord.map/contains.pass.cpp:17
 
 // bool contains(const key_type& x) const;
 
----------------
Can you please create a separate `contains.transparent.pass.cpp` test? Same for other methods.


================
Comment at: libcxx/test/std/containers/unord/unord.map/contains.pass.cpp:64
+      // Make sure conversions don't happen for transparent non-final hasher and key_equal
+      typedef const std::unordered_map<Comp2Int, int, transparent_hash,
+                                       std::equal_to<> >
----------------
You can use `using XX = ...` here to make the line wrapping less weird. (also in other tests)


================
Comment at: libcxx/test/std/containers/unord/unord.map/count.pass.cpp:24
+#include "test_transparent_unordered.h"
+#include <iostream>
 
----------------
This include can (and should) be removed.


================
Comment at: libcxx/test/support/is_transparent.h:102
+
+struct Comp2Int { // convertible from int
+  Comp2Int() : i_(0) {}
----------------
Could you make this maintain a pointer to an `int` stored externally instead of having a static data member? This will make it easier to port the test to `constexpr` in the future. I've had to fix a lot of those while working on constexpr allocators and vector :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87171