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

[Ruslan Arutyunyan via Phabricator via libcxx-commits]

rarutyun marked 4 inline comments as done and an inline comment as not done.
rarutyun added inline comments.


================
Comment at: libcxx/include/unordered_map:473
+#if _LIBCPP_STD_VER > 17
+    template <typename _K2>
+    _LIBCPP_INLINE_VISIBILITY
----------------
ldionne wrote:
> 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.
I applied `EnableIf` pattern with dummy template parameter on the private interfaces and used `_EnableIf` with return type for public methods. 


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




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

https://reviews.llvm.org/D87171