[libcxx-commits] [PATCH] D116621: [libc++][P2321R2] Add const overloads to tuple swap, construction and assignment

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/tuple:848
     struct _EnableImplicitCopyFromPair : _And<
-        is_constructible<_FirstType<_DependentTp...>, const _Up1&>,
-        is_constructible<_SecondType<_DependentTp...>, const _Up2&>,
----------------
ldionne wrote:
> EricWF wrote:
> > These bits of SFINAE were written very specifically because tuple is a very weird beast.  
> > 
> > I'm surprised we don't have a test in the test suite that breaks after changing this, because I thought I added one.
> > 
> > There are certain SFINAE loops that crop up where checking these properties in the specific order is/was important.
> > 
> I think what Eric's saying here is that you need to test for `_EnableCopyFromPair` *before* you test the two `is_convertible` conditions.
> 
> I think you could witness the difference by using a type that is *not* constructible from `_Up`, but where a conversion from `_Up` triggers a hard error. In that case, the fact that you changed the order of checking will cause you to instantiate the conversion operator before the constructor, and explode. If you had checked for the constructibility first, you'd have gotten a false answer and stopped there, instead of being tripped up by the hard error later on while trying to check whether the constructor should be explicit or not.
> 
> Please add tests for this -- we actually see these sorts of funky bad-behaved constructors and conversions in the wild. I remember breaking `nlohmann::json` with a change a couple years ago -- it wasn't fun. I'm not sure whether it ended up being our fault our theirs, but regardless, we want to be bullet proof to avoid breaking widely used libraries, because we look like jokers when that happens :-).
I fixed the ordering issue from the previous version. But I am not sure If I understand the test case. IIUC, you are suggesting a case where
std::is_constructible_v<T, U> is false, but
std::is_convertible_v<U, T> is hard error

IIUC, convertible is a stricter version of constructible. if is_convertible_v has a hard error, the conversion operator/constructor that causes the error would also cause a hard error when evaluating is_constructible. or did I miss something?


================
Comment at: libcxx/include/tuple:1115
+    _LIBCPP_HIDE_FROM_ABI constexpr
+    const tuple& operator=(_If<_And<is_assignable<_Tp&&, _Tp&&>...>::value, tuple, __nat>&& __tuple) const {
+        std::__memberwise_forward_assign(*this,
----------------
EricWF wrote:
> I don't know that we need to do the `_If` trick here. Because it's not the true "copy assignment operator", we don't need to worry about the compiler generating one.
> 
> Try writing these like normal overloads with normal SFINAE.
For this one, I tempt to keep `_If` as it is. few reasons:

1. This function does not naturally have a deduced type that can be used by SFINAE
2. I could introduce some template parameters, but that needs to have default value with `Tp...`. I am not sure if that is going to be very readable. Also, it will make it look like the overload that takes `tuple<Us...>`
3. The best way I think is to use `requires` but I am not sure if it is OK to use it as the rest of (C++20 bit) the file does not use it



================
Comment at: libcxx/include/tuple:1125
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
     tuple& operator=(_If<_And<is_move_assignable<_Tp>...>::value, tuple, __nat>&& __tuple)
         _NOEXCEPT_((_And<is_nothrow_move_assignable<_Tp>...>::value))
----------------
EricWF wrote:
> Are we implementing this for `std::array` too?
I am not sure if it is going to be very useful. The main use case (at least for the purpose of p2321r2) for these `const` overloads of assignment operators is that we can assign `const tuple` of reference types. (well, the const value types usually cannot be assigned to).  I don't think `std::array` supports references


================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/convert_cpp23.pass.cpp:16
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// XFAIL: gcc
+
----------------
EricWF wrote:
> ldionne wrote:
> > Why does it fail on GCC?
> Oh, good catch.
> 
> I suspect it's because GCC rejects mutating mutable members in constant expressions.
> 
> Since we want *some* test coverage on GCC, I suggest we find a way to make at least part of this test pass under it.
I disabled running these tests on compile time on gcc. but at least, it would run the tests at runtime with gcc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116621