[libcxx-commits] [PATCH] D143914: [libc++] Clean up pair's constructors and assignment operators

[Tim Song via Phabricator via libcxx-commits]

tcanens added inline comments.


================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv.pass.cpp:80
+    static_assert(!std::is_assignable_v<std::pair<long, std::string>&, std::tuple<long, void*>>); // second not convertible
+    static_assert( std::is_assignable_v<std::pair<long, std::string>&, std::tuple<long, std::string>>); // works (test the test)
+  }
----------------
ldionne wrote:
> @huixie90 @tcanens 
> 
> While implementing this, I found out that stuff like this would compile just fine:
> 
> ```
> std::pair<long, std::string> p;
> p = std::tuple<long, float>{};
> ```
> 
> This is because `std::string` can be assigned-to from `float`. While this has nothing to do with `pair` specifically, this seems completely bonkers to me -- this is almost Javascript bad. I'm curious to know whether that has been discussed and acknowledged?
See https://cplusplus.github.io/LWG/issue3311 and the duplicate.




================
Comment at: libcxx/test/std/utilities/utility/pairs/pairs.pair/assign.pair_like_rv_const.pass.cpp:57
+
+    // Make sure it doesn't work for `ranges::subrange`, since it is called out explicitly.
+    {
----------------
ldionne wrote:
> @tcanens @huixie90 
> 
> This behavior seems to be inconsistent with the non-const-qualified `operator=` from `pair-like`. For `operator=(PairLike)` (not `const`), it seems like we go through `subrange`'s `operator pair-like()`, which makes this work. But with `operator=(pair-like) const`, it looks like this conversion doesn't trigger and we end up not being able to perform the assignment. Does that match the LWG design intent?
It doesn't trigger because `ConstAssignable` is not convertible from `int*`? Are the const/non-const cases actually different?

IIRC the constraint is there to prevent bypassing the slicing check on subrange's `operator pair-like`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143914