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

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun added a comment.

Hi @zoecarver. I am also sorry about duplicating the work. I looked at the cxx2a_status.html and saw that it is not taken. I created the review with just having the update of cxx2a_status.html with marking that feature as //In progress//. Unfortunately, I didn't have information who should be invited to the review. I knew only Marshall and added him. Seems nobody was informed I took this part of work. As you can see the review was created on September the 5th. After that it was very hard to find the time to finish the feature implementation. Finally I found the bandwidth and implemented all of this stuff for a couple of days.

Thanks to agree to close your patch. I hope the process will become clearer to me and I hope we will avoid such situations in future. This is my first patch to libc++ and I definitely want to deliver it.

Yes, you are right the correct behavior is just do not allow the methods to participate in overload resolution. `static_assert` doesn't provide the desired behavior. Instead, user may see the compile-time error in places where it (error) should not happen.

@ldionne, sure I will look at D59886 <https://reviews.llvm.org/D59886> and try to figure out what tests I can take from there. And regarding `__cpp_lib_generic_unordered_lookup` feature macro question. At least, I didn't add it. If it should be added let me figure out how to do that.



================
Comment at: libcxx/include/unordered_map:473
+#if _LIBCPP_STD_VER > 17
+    template <typename _K2>
+    _LIBCPP_INLINE_VISIBILITY
----------------
ldionne wrote:
> 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`.
> 
Personally I don't like the approach with dummy argument since it adds possibility to use interface in the wrong way. Furthermore, It doesn't work in all the cases. I definitely can do that for those internal interfaces and I don't see the problems with that. But if you allow me I would prefer to have the `enable_if_t` as the return type for public interfaces (like `find`, `contains`, etc.) to do not have such a dummy parameter on the user level. If prevents incorrect usage and makes code completion hints in IDEs clearer.


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


================
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<> >
----------------
ldionne wrote:
> You can use `using XX = ...` here to make the line wrapping less weird. (also in other tests)
Yes, I also had that in mind. That would make things easier. I decided to finish the patch before this improvement. But I agree. Thanks for the suggestion. I am also considering to add common template alias with the template-template parameter as the first place to just pass unordered map or set. I want to look how if would be in the code and decide if such a common alias is acceptable or not


================
Comment at: libcxx/test/std/containers/unord/unord.map/count.pass.cpp:24
+#include "test_transparent_unordered.h"
+#include <iostream>
 
----------------
ldionne wrote:
> This include can (and should) be removed.
Right. Thanks. I noticed this after submitting a patch.


================
Comment at: libcxx/test/support/is_transparent.h:102
+
+struct Comp2Int { // convertible from int
+  Comp2Int() : i_(0) {}
----------------
ldionne wrote:
> 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 :)
I think I understand the idea. Sure, I can make that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87171