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

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

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


================
Comment at: libcxx/test/support/is_transparent.h:102
+
+struct Comp2Int { // convertible from int
+  Comp2Int() : i_(0) {}
----------------
ldionne wrote:
> rarutyun wrote:
> > ldionne wrote:
> > > 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`.
> > > 
> > I actually thought about that in a different manner. But what I had in mind also doesn't help with the `constexpr` use-case I guess.
> > 
> > With what you suggest I don't understand how to make `Comp2Int` class implicitly constructible from `int`. I just pass one `int` value to the `find` or `count` (or whatever) interface. This `int` argument won't match the constructor with two arguments.
> > 
> > 
> Ah, yeah, that makes it impossible. If you're using `int`.
> 
> However, what we can do is:
> 
> ```
> template <typename T>
> struct CountConversions {
>   CountConversions(T value, int* counter) : ... { }
>   operator T() const { ++*conversions_; return value_; }
> 
> private:
>   T value_;
>   int* conversions_;
> };
> 
> template <typename T>
> struct ComparableTo {
>   ComparableTo() = default;
>   ComparableTo(T value) : value_(value) { }
> 
>   friend bool operator==(T const& lhs, ComparableTo const& rhs) {
>     return lhs == rhs.value_;
>   }
> 
>   friend bool operator==(ComparableTo const& lhs, T const& rhs) {
>     return lhs.value_ == rhs;
>   }
> 
>   friend bool operator==(ComparableTo const& lhs, ComparableTo const& rhs) {
>     return lhs.value_ == rhs.value_;
>   }
> 
> private:
>   T value_;
> };
> ```
> 
> Then, use:
> 
> ```
> std::unordered_set<ComparableTo<int>, transparent_hash, std::equal_to<>> set = {
>   ComparableTo<int>(1),
>   ComparableTo<int>(2),
>   ComparableTo<int>(3)
> };
> 
> int conversions = 0;
> assert(c.contains(CountConversions(1, &conversions)));
> assert(c.contains(CountConversions(2, &conversions)));
> assert(conversions == 0);
> assert(!c.contains(CountConversions(4, &conversions)));
> assert(conversions == 0);
> ```
> 
> I haven't tested this, but I think it should work.
> 
No, it doesn't. You still have conversions in the scenario above. If you create the `unordered_set<ComparableTo<int>>` then the `T = int` then the hidden friends have `int` as well. When the heterogeneous API is called it calls `operator==` that causes `CountConversions` to be converted to `int` and it makes `conversions_` to be incremented. Of course the test fails.

I think that if we create `ComparableTo<CountConversions<int>>` that might actually work but you need to teach the `operator==` to work with such types. But the biggest problem is you should create `int conversions` variable in places where you want to initialize the container but you don't need it, it's just dummy parameter to initialize the container of `ComparableTo<CountConversions<int>>`.

Having all of that in mind I made a step further and seems like I found the way how to test the containers with passing external reference counter. I uploaded the patch with that approach. It allows to avoid dummy `int conversions` definition when container is initialized. It also tests the constructor of `KeyType`, which IMO better fits the testing expectations rather then conversion operator of heterogeneous `Key`.


================
Comment at: libcxx/test/support/is_transparent.h:107
+  static void reset_conversions_count() { conversions_count = 0; }
+  operator int() const noexcept { return i_; }
+
----------------
ldionne wrote:
> Is this necessary for testing, or only to implement the comparison operators below and the hashing above? If the latter, I would much rather remove this as it makes things harder to understand -- this type focuses on being comparable with `int`, and constructible from `int`.
> 
> Edit: Please try using the definitions I've provided above and let me know if that works -- if it does, I think it would be simpler and wouldn't require a conversion operator for the type stored inside the container.
Yes, it was necessary to implement hash above and allow this type to be used in `std::hash<int>`. Due to all of these wrappers I've added we cannot use `std::hash<int>` anyway. But we still need to calculate the hash for both `ComparableTo` and `CounterWrapper`. So, I added `get()` method to both. The other approach is `friend` but I don't think that it's better


================
Comment at: libcxx/test/support/test_transparent_unordered.h:20
+          typename Equal>
+using unord_set_type = UnorderedMap<Comp2Int, Hash, Equal>;
+
----------------
ldionne wrote:
> Now I understand what you meant in your previous comment. I don't feel like these aliases buy us a lot of readability -- I would prefer removing them.
> 
> Also, they seem to be reversed (`unord_set_type = UnorderedMap<...>` seems wrong).
Yes, thanks for noticing. It's just name swapping. I probably agree that they don't bring much readability but what the improve is maintainability. I was needed to rewrite the tests on the new approach. Those aliases helped me to do that easier. I would prefer to leave them. They also help to change `KeyEqual` and `Hash` types without listing others.

If you insist I may remove them but my opinion that they bring value.


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

https://reviews.llvm.org/D87171