[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:858
-        _Not<is_convertible<const _Up1&, _FirstType<_DependentTp...> > >, // explicit check
-        _Not<is_convertible<const _Up2&, _SecondType<_DependentTp...> > >
-    > { };
----------------
ldionne wrote:
> Do we need `_SecondType` and `_FirstType` in `<type_traits>` anymore? If not, let's remove them.
They were used in the `noexcept` specifier of some constructors and assignment operators. I went ahead refactoring these functions and `_FirstType` and `_SecondType` are no longer used


================
Comment at: libcxx/include/tuple:1178-1185
+    template <bool _Const, class _Pair, class _DecayedPair = __uncvref_t<_Pair>, class _Tuple = tuple>
+    struct _EnableAssignFromPair : false_type {};
+
+    template <bool _Const, class _Pair, class _Up1, class _Up2, class _Tp1, class _Tp2>
+    struct _EnableAssignFromPair<_Const, _Pair, pair<_Up1, _Up2>, tuple<_Tp1, _Tp2>> : 
+        _And<is_assignable<__maybe_const<_Const, _Tp1>&, __copy_cvref_t<_Pair, _Up1>>,
+             is_assignable<__maybe_const<_Const, _Tp2>&, __copy_cvref_t<_Pair, _Up2>>
----------------
ldionne wrote:
> I think we should either use `_EnableAssignFromPair` with the const **and** the non-const overloads, or simplify `_EnableAssignFromPair` to always assume that `_Const == true`. Otherwise, this code looks buggy, even though it isn't.
I did the former as refactoring the non-const allows me to completely remove  _FirstType


================
Comment at: libcxx/include/tuple:848
     struct _EnableImplicitCopyFromPair : _And<
-        is_constructible<_FirstType<_DependentTp...>, const _Up1&>,
-        is_constructible<_SecondType<_DependentTp...>, const _Up2&>,
----------------
ldionne wrote:
> huixie90 wrote:
> > 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?
> I think I was thinking about a type having a conversion operator that triggers a hard error. However, it doesn't seem to apply anymore, but it would be nice to ensure that we don't trip over a type `T` where `is_constructible<T, U>::value` is false, but where `is_convertible<T, U>::value` is a hard error (by defining a conversion operator from `U` to `T` that causes a hard error). Let me know if you think this doesn't make sense, it might be impossible to actually trigger this issue but I'd need to dive deeper to be certain.
hmm. IIUC, `is_constructible` also looks for conversion operator. If a conversion operator from `U` to `T` causes a hard error, both `is_convertible` and `is_constructible` would cause hard error. or, did I miss anything?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116621