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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D87171#2322821 <https://reviews.llvm.org/D87171#2322821>, @zoecarver wrote:

> In D87171#2322740 <https://reviews.llvm.org/D87171#2322740>, @rarutyun wrote:
>
>> 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.
>
> Just add a `#define __cpp_lib_generic_unordered_lookup 202010L` in `<version>`.

Oops, that's incorrect! Instead, look for `__cpp_lib_generic_unordered_lookup` in `libcxx/utils/generate_feature_test_macro_components.py`. We already have that feature test macro in the script, but it's disabled. Enable it and run the script, you'll see the magic happen!

Also, please rebase on top of `master` -- I updated the `generate_feature_test_macro_components.py` with one fix that'll make your life easier.



================
Comment at: libcxx/include/unordered_map:473
+#if _LIBCPP_STD_VER > 17
+    template <typename _K2>
+    _LIBCPP_INLINE_VISIBILITY
----------------
zoecarver wrote:
> rarutyun wrote:
> > 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.
> Where possible, I suggest using `_EnableIf`. We can optimize that internally to improve compile times. 
Ok to leave as-is. As Zoe suggests, you can use `_EnableIf` instead.


================
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<> >
----------------
rarutyun wrote:
> 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
It's acceptable if it makes the code easier to follow!


================
Comment at: libcxx/test/support/is_transparent.h:102
+
+struct Comp2Int { // convertible from int
+  Comp2Int() : i_(0) {}
----------------
rarutyun wrote:
> 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.
Basically, it would look like this:

```
struct Comp2Int { // convertible from int
  Comp2Int(int* conversions_count) : i_(0), conversions_count_(conversions_count) {}
  Comp2Int(int i, int* conversions_count) : i_(i), conversions_count_(conversions_count) { ++*conversions_count_; }
  int get_conversions_count() { return *conversions_count_; }
  operator int() const noexcept { return i_; }

private:
  int* conversions_count_;
  int i_;
};
```

`reset_conversions_count` should not be relevant anymore.

Then, from the tests, you create a local `int conversions = 0` and pass the address of that to `Comp2Int`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87171