[libcxx-commits] [PATCH] D116621: [libc++][P2321R2] Add const overloads to tuple swap, construction and assignment
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Jun 7 08:21:58 PDT 2022
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
I have some comments, but I really like the current state of this patch. This implements part of a new paper while refactoring a bunch of stuff and making it simpler at once, which is really great. Requesting changes because I'd like to take a look after comments have been applied, but I think this looks really good. Also, the tests are nicely structured and seem to cover all the required cases except for perhaps one minor detail mentioned in the comments. Thanks!
================
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,
----------------
huixie90 wrote:
> 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
>
Per the above comment, we can use `requires` here and mark this original comment as done, IMO.
================
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))
----------------
huixie90 wrote:
> 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
Also, note that we don't support assigning `tuple = array` anymore. This was an extension, but it was removed.
================
Comment at: libcxx/include/tuple:30
+ template<class... UTypes>
+ constexpr explicit(see below) tuple(tuple<UTypes...>&); // C++23
template <class... U>
----------------
Nit, but let's be consistent. Here and below.
================
Comment at: libcxx/include/tuple:779
+ template <class _OtherTuple, class _DecayedOtherTuple = __uncvref_t<_OtherTuple>, class = void>
+ struct _EnableCtrFromUTypesTuple : false_type {};
+
----------------
Really small nitpick, but we generally use `ctor` instead of `ctr` for constructor (at least it's what I've seen so far).
================
Comment at: libcxx/include/tuple:784
+ // the length of the packs needs to checked first otherwise the 2 packs cannot be expanded simultaneously below
+ typename enable_if<sizeof...(_Up) == sizeof...(_Tp)>::type> : _And<
+ _Not<is_same<_OtherTuple, const tuple&> >, // not in spec, but to keep previous libcxx behaviour
----------------
================
Comment at: libcxx/include/tuple:785-786
+ typename enable_if<sizeof...(_Up) == sizeof...(_Tp)>::type> : _And<
+ _Not<is_same<_OtherTuple, const tuple&> >, // not in spec, but to keep previous libcxx behaviour
+ _Not<is_same<_OtherTuple, tuple&&> >, // not in spec, but to keep previous libcxx behaviour
+ is_constructible<_Tp, __copy_cvref_t<_OtherTuple, _Up> >...,
----------------
What happens if you remove these conditions? The comments should explain what is the behavior that we are preserving.
================
Comment at: libcxx/include/tuple:788
+ is_constructible<_Tp, __copy_cvref_t<_OtherTuple, _Up> >...,
+ _Or<_BoolConstant<sizeof...(_Tp) != 1>,
// _Tp and _Up are 1-element packs - the pack expansions look
----------------
================
Comment at: libcxx/include/tuple:791
// weird to avoid tripping up the type traits in degenerate cases
- _Lazy<_And,
- _Not<is_convertible<const tuple<_Up>&, _Tp> >...,
- _Not<is_constructible<_Tp, const tuple<_Up>&> >...
+ _And<
+ _Not<is_same<_Tp, _Up> >...,
----------------
I'm not requesting it, however if you'd like to add a regression test where for example `_BoolConstant<sizeof...(_Tp) != 1>` is false, but evaluating `is_constructible<_Tp, _OtherTuple>` is a hard error, I think it would catch this.
================
Comment at: libcxx/include/tuple:793-794
+ _Not<is_same<_Tp, _Up> >...,
+ _Lazy<_Not, is_convertible<_OtherTuple, _Tp> >...,
+ _Lazy<_Not, is_constructible<_Tp, _OtherTuple> >...
>
----------------
Mentioning `_Not<T>` does not instantiate `T` immediately, it only does when you do `_Not<T>::whatever`. Since you only do that lazily with my suggested edits to `_And` and `_Or`, you don't need `_Lazy` at this level anymore -- at least I think you don't, but our tests should tell you if I'm right.
================
Comment at: libcxx/include/tuple:858
- _Not<is_convertible<const _Up1&, _FirstType<_DependentTp...> > >, // explicit check
- _Not<is_convertible<const _Up2&, _SecondType<_DependentTp...> > >
- > { };
----------------
Do we need `_SecondType` and `_FirstType` in `<type_traits>` anymore? If not, let's remove them.
================
Comment at: libcxx/include/tuple:949
+ _EnableCtrFromPair<const pair<_Up1, _Up2>&>,
+ _BothImplicitlyConvertible<const pair<_Up1, _Up2>&>
>::value
----------------
It's pretty obvious, but just to stay consistent with the other constructors above. This also applies to the other constructors below.
================
Comment at: libcxx/include/tuple:1097
+ _LIBCPP_HIDE_FROM_ABI constexpr
+ const tuple& operator=(_If<_And<is_assignable<const _Tp&, const _Tp&>...>::value, tuple, __nat> const& __tuple) const {
+ std::__memberwise_copy_assign(*this, __tuple, typename __make_tuple_indices<sizeof...(_Tp)>::type());
----------------
We could use `requires` instead since we are in C++ > 20 and we only support compilers that support concepts.
================
Comment at: libcxx/include/tuple:1156
+ template <class... _UTypes, enable_if_t<
+ _And<_BoolConstant<sizeof...(_Tp) == sizeof...(_UTypes)>,
+ is_assignable<const _Tp&, const _UTypes&>...>::value>* = nullptr>
----------------
I like that you use `_BoolConstant<sizeof...(_Tp) == sizeof...(_UTypes)>` for self-documentation of the condition, however technically it is not needed because `is_assignable<const _Tp&, const _UTypes&>...` would SFINAE-away if that were not the case. I suggest you leave as-is for documentation purposes, I just wanted to point it out.
================
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>>
----------------
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.
================
Comment at: libcxx/include/tuple:1312
+ _LIBCPP_HIDE_FROM_ABI constexpr void swap(const tuple&) const noexcept {}
+#endif // _LIBCPP_STD_VER > 20
};
----------------
IMO it's not necessary for such a small block.
================
Comment at: libcxx/include/tuple:1361
+}
+#endif // _LIBCPP_STD_VER > 20
+
----------------
Ditto.
================
Comment at: libcxx/include/tuple:848
struct _EnableImplicitCopyFromPair : _And<
- is_constructible<_FirstType<_DependentTp...>, const _Up1&>,
- is_constructible<_SecondType<_DependentTp...>, const _Up2&>,
----------------
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.
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_convert_copy.pass.cpp:34-35
+// (is_assignable_v<const Types&, const UTypes&> && ...) is true
+static_assert(
+ std::is_assignable_v<const std::tuple<AssignableFrom<ConstCopyAssign>>&, const std::tuple<ConstCopyAssign>&>);
+
----------------
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_convert_copy.pass.cpp:65
+
+ // correct overload should be selected
+ {
----------------
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_convert_copy.pass.cpp:80
+// gcc cannot have mutable member in constant expression
+#if !defined(__GNUG__) || defined(__clang__)
+ static_assert(test());
----------------
You'll want to `#include "test_macros.h"` to get this.
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_pair_copy.pass.cpp:15
+// - sizeof...(Types) is 2,
+// - is_assignable_v<const T0&, const U1&> is true, and
+// - is_assignable_v<const T1&, const U2&> is true
----------------
Would it make sense to use `T1, T2` instead of `T0, T1`? That way, indexing would match for `Ti` and `Ui`? If so, we should update the comments below.
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/const_pair_copy.pass.cpp:41
+
+// - is_assignable_v<const T1&, const U2&> is false
+static_assert(!std::is_assignable_v<const std::tuple<AssignableFrom<ConstCopyAssign>, AssignableFrom<CopyAssign>>&,
----------------
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/types.h:1
+#ifndef __TEST_TUPLE_ASSIGN_TYPES_H
+#define __TEST_TUPLE_ASSIGN_TYPES_H
----------------
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.assign/types.h:3
+#define __TEST_TUPLE_ASSIGN_TYPES_H
+
+#include "test_allocator.h"
----------------
This file is missing a license :)
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_const_move.pass.cpp:114
+ // These two test points cause gcc to ICE
+#if !defined(__GNUG__) || defined(__clang__)
+ // sizeof...(Types) == 1 && is_convertible_v<decltype(u), T>
----------------
Let's use `TEST_COMPILER_GCC`.
================
Comment at: libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/convert_types.h:1
+#ifndef __TEST_TUPLE_CNSTR_TYPES_H
+#define __TEST_TUPLE_CNSTR_TYPES_H
----------------
Same comment for license and header guard.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116621/new/
https://reviews.llvm.org/D116621
More information about the libcxx-commits
mailing list