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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/support/is_transparent.h:102
+
+struct Comp2Int { // convertible from int
+  Comp2Int() : i_(0) {}
----------------
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.



================
Comment at: libcxx/test/support/is_transparent.h:107
+  static void reset_conversions_count() { conversions_count = 0; }
+  operator int() const noexcept { return i_; }
+
----------------
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.


================
Comment at: libcxx/test/support/test_transparent_unordered.h:20
+          typename Equal>
+using unord_set_type = UnorderedMap<Comp2Int, Hash, Equal>;
+
----------------
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).


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

https://reviews.llvm.org/D87171