[libcxx] makes pair conditionally trivially copyable for C++20
#84811
Conversation
cjdb
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Christopher Di Bella (cjdb) Changes
It's currently unclear how to implement this without using a requires-clause, so the feature is restricted to C++20 for now, but backporting this to C++11 would be ideal. There has also been some interest in moving this out of the unstable ABI mode. Both of these changes are intended to arrive in future pull requests, should their feasibility pan out. Full diff: https://github.com/llvm/llvm-project/pull/84811.diff 5 Files Affected:
diff --git a/libcxx/include/__config b/libcxx/include/__config
index 11e13e0c24986a..a8720cd76ca03d 100644
--- a/libcxx/include/__config
+++ b/libcxx/include/__config
@@ -180,6 +180,9 @@
// requires code not to make these assumptions.
# define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_ARRAY
# define _LIBCPP_ABI_USE_WRAP_ITER_IN_STD_STRING_VIEW
+// Allows std::pair's copy assignment and move assignment operators to be trivial
+// when both its element types' respective assignment operators are trivial.
+# define _LIBCPP_ABI_PAIR_TRIVIALLY_COPYABLE
# elif _LIBCPP_ABI_VERSION == 1
# if !(defined(_LIBCPP_OBJECT_FORMAT_COFF) || defined(_LIBCPP_OBJECT_FORMAT_XCOFF))
// Enable compiling copies of now inline methods into the dylib to support
diff --git a/libcxx/include/__utility/pair.h b/libcxx/include/__utility/pair.h
index b488a9829c3849..7cded19bcf694a 100644
--- a/libcxx/include/__utility/pair.h
+++ b/libcxx/include/__utility/pair.h
@@ -26,20 +26,24 @@
#include <__type_traits/common_type.h>
#include <__type_traits/conditional.h>
#include <__type_traits/decay.h>
+#include <__type_traits/enable_if.h>
#include <__type_traits/integral_constant.h>
#include <__type_traits/is_assignable.h>
#include <__type_traits/is_constructible.h>
#include <__type_traits/is_convertible.h>
#include <__type_traits/is_copy_assignable.h>
+#include <__type_traits/is_trivially_copy_assignable.h>
#include <__type_traits/is_default_constructible.h>
#include <__type_traits/is_implicitly_default_constructible.h>
#include <__type_traits/is_move_assignable.h>
+#include <__type_traits/is_trivially_move_assignable.h>
#include <__type_traits/is_nothrow_assignable.h>
#include <__type_traits/is_nothrow_constructible.h>
#include <__type_traits/is_nothrow_copy_assignable.h>
#include <__type_traits/is_nothrow_copy_constructible.h>
#include <__type_traits/is_nothrow_default_constructible.h>
#include <__type_traits/is_nothrow_move_assignable.h>
+#include <__type_traits/is_object.h>
#include <__type_traits/is_same.h>
#include <__type_traits/is_swappable.h>
#include <__type_traits/nat.h>
@@ -67,6 +71,14 @@ struct __non_trivially_copyable_base {
__non_trivially_copyable_base(__non_trivially_copyable_base const&) _NOEXCEPT {}
};
+#if _LIBCPP_STD_VER >= 20
+template<class _Tp>
+concept __trivially_copy_assignable_object = is_trivially_copy_assignable_v<_Tp> && is_object_v<_Tp>;
+
+template<class _Tp>
+concept __trivially_move_assignable_object = is_trivially_move_assignable_v<_Tp> && is_object_v<_Tp>;
+#endif
+
#if _LIBCPP_STD_VER >= 23
template <class _Tp>
struct __is_specialization_of_subrange : false_type {};
@@ -236,6 +248,13 @@ struct _LIBCPP_TEMPLATE_VIS pair
typename __make_tuple_indices<sizeof...(_Args1)>::type(),
typename __make_tuple_indices<sizeof...(_Args2) >::type()) {}
+# if _LIBCPP_STD_VER >= 20 && defined(_LIBCPP_ABI_PAIR_TRIVIALLY_COPYABLE)
+ _LIBCPP_HIDE_FROM_ABI pair& operator=(pair const& __p)
+ requires __trivially_copy_assignable_object<first_type> && __trivially_copy_assignable_object<second_type> = default;
+ _LIBCPP_HIDE_FROM_ABI pair& operator=(pair&& __p)
+ requires __trivially_move_assignable_object<first_type> && __trivially_move_assignable_object<second_type> = default;
+#endif
+
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pair&
operator=(__conditional_t< is_copy_assignable<first_type>::value && is_copy_assignable<second_type>::value,
pair,
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp
index 253bd1b85d03ec..f45903f2a263a7 100644
--- a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp
+++ b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp
@@ -88,6 +88,16 @@ TEST_CONSTEXPR_CXX20 bool test() {
assert(&p.second == &inc_obj);
}
+#if TEST_STD_VER >= 20 && defined(_LIBCPP_ABI_PAIR_TRIVIALLY_COPYABLE)
+ static_assert(std::is_trivially_copy_assignable<std::pair<int, int>>::value, "");
+#else
+ static_assert(!std::is_trivially_copy_assignable<std::pair<int, int>>::value, "");
+#endif
+
+ static_assert(!std::is_trivially_copy_assignable<std::pair<CountAssign, int>>::value, "");
+ static_assert(!std::is_trivially_copy_assignable<std::pair<int, CountAssign>>::value, "");
+ static_assert(!std::is_trivially_copy_assignable<std::pair<CountAssign, CountAssign>>::value, "");
+
return true;
}
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair_cxx03.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair_cxx03.pass.cpp
index 2444131b02a0cf..baa472e1cf4525 100644
--- a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair_cxx03.pass.cpp
+++ b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair_cxx03.pass.cpp
@@ -62,6 +62,8 @@ int main(int, char**)
assert(p.second == 'x');
}
+ static_assert(!std::is_trivially_copy_assignable<std::pair<int, int>>::value);
+
return 0;
}
diff --git a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp
index 8e5d9c39ae8868..a4c178db25492a 100644
--- a/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp
+++ b/libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cpp
@@ -130,6 +130,17 @@ TEST_CONSTEXPR_CXX20 bool test() {
static_assert(!std::is_move_assignable<P5>::value, "");
static_assert(!std::is_move_assignable<P6>::value, "");
}
+
+#if TEST_STD_VER >= 20 && defined(_LIBCPP_ABI_PAIR_TRIVIALLY_COPYABLE)
+ static_assert(std::is_trivially_move_assignable<std::pair<int, int>>::value, "");
+#else
+ static_assert(!std::is_trivially_move_assignable<std::pair<int, int>>::value, "");
+#endif
+
+ static_assert(!std::is_trivially_move_assignable<std::pair<CountAssign, int>>::value, "");
+ static_assert(!std::is_trivially_move_assignable<std::pair<int, CountAssign>>::value, "");
+ static_assert(!std::is_trivially_move_assignable<std::pair<CountAssign, CountAssign>>::value, "");
+
return true;
}
|
You can test this locally with the following command:git-clang-format --diff 725a0523a18ef1a75a6d4a010dc3debe1b08c9d1 f2ef4fd60ab3686644d81c1d695c9b702e3a874f -- libcxx/test/std/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp libcxx/include/__config libcxx/include/__utility/pair.h libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair.pass.cpp libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_pair_cxx03.pass.cpp libcxx/test/std/utilities/utility/pairs/pairs.pair/assign_rv_pair.pass.cppView the diff from clang-format here.diff --git a/libcxx/include/__utility/pair.h b/libcxx/include/__utility/pair.h
index fb38f29e90..6438664210 100644
--- a/libcxx/include/__utility/pair.h
+++ b/libcxx/include/__utility/pair.h
@@ -251,11 +251,12 @@ struct _LIBCPP_TEMPLATE_VIS pair
# if _LIBCPP_STD_VER >= 20 && defined(_LIBCPP_ABI_PAIR_TRIVIALLY_COPYABLE)
_LIBCPP_HIDE_FROM_ABI pair& operator=(pair const& __p) = default;
- _LIBCPP_HIDE_FROM_ABI pair& operator=(pair&& __p) = default;
+ _LIBCPP_HIDE_FROM_ABI pair& operator=(pair&& __p) = default;
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pair& operator=(pair const& __p)
_NOEXCEPT_(is_nothrow_copy_assignable<first_type>::value&& is_nothrow_copy_assignable<second_type>::value)
- requires is_copy_assignable_v<first_type> && is_copy_assignable_v<second_type> && (!(__trivially_copy_assignable_object<first_type> && __trivially_copy_assignable_object<second_type>))
+ requires is_copy_assignable_v<first_type> && is_copy_assignable_v<second_type> &&
+ (!(__trivially_copy_assignable_object<first_type> && __trivially_copy_assignable_object<second_type>))
{
first = __p.first;
second = __p.second;
@@ -264,7 +265,8 @@ struct _LIBCPP_TEMPLATE_VIS pair
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pair& operator=(pair&& __p)
_NOEXCEPT_(is_nothrow_move_assignable<first_type>::value&& is_nothrow_move_assignable<second_type>::value)
- requires is_move_assignable_v<first_type> && is_move_assignable_v<second_type> && (!(__trivially_move_assignable_object<first_type> && __trivially_move_assignable_object<second_type>))
+ requires is_move_assignable_v<first_type> && is_move_assignable_v<second_type> &&
+ (!(__trivially_move_assignable_object<first_type> && __trivially_move_assignable_object<second_type>))
{
first = std::forward<first_type>(__p.first);
second = std::forward<second_type>(__p.second);
|
|
Isn't this an ODR violation when compiling pre-C++20 code and C++20 code in the unstable ABI? Why didn't the return type SFINAE not work out? |
Technically, yes, but I wonder if it's in the same vein as the ABI break we talked about. Is this a concern for our unstable ABI users? If it is, is there a reason folks compiling different parts of their binary aren't able to disable this particular feature? @jyknight might have more insight into this from a practical perspective.
SFINAE only works in dependent contexts, and I don't think SMFs can ever be dependent. // Not SFINAE
auto operator=(pair const&)
-> enable_if<is_trivially_copy_assignable_v<T> && is_trivially_copy_assignable_v<U>> = default;
// Not a copy assignment operator; can't be defaulted
pair& operator=(__conditional_t<is_trivially_copy_assignable_v<X> && is_trivially_copy_assignable_v<Y>,
pair,
__nat> const&) = default;
// Not a copy assignment operator; can't be defaulted
template<class X = T, class Y = U>
auto operator=(pair const&)
-> enable_if<is_trivially_copy_assignable_v<X> && is_trivially_copy_assignable_v<Y>> = default;
// defaulted constructors seem to preclude this approach
pair& operator=(__conditional_t<is_trivially_copy_assignable_v<X> && is_trivially_copy_assignable_v<Y>,
__nat,
pair> const&) = delete;
template<bool = true>
pair& operator=(__conditional_t<!(is_trivially_copy_assignable_v<X> && is_trivially_copy_assignable_v<Y>),
pair,
__nat> const&) { ... } |
|
|
I think it's fine. I just wanted to call it out so we're all on the same page about this.
Urgh, right. Maybe we could handle it through a base class that is templated on whether we can default the assignment? |
`pair` can have trivial copy and move assignment operators when both its members have trivial copy and move operators, which opens `pair` up to being trivially copyable. This is the first of three possible commits to bring users a more robust implementation. It's currently unclear how to implement this without using a _requires-clause_, so the feature is restricted to C++20 for now. There has also been some interest in moving this out of the unstable ABI mode. Both of these changes are intended to arrive in future pull requests, should their feasibility pan out.
It seems that |
How much of an impact would this approach have on the ABI? |
|
I believe there shouldn't be ABI impact from modifying pair to derive from a zero-sized base class. |
I'll rework it in that case :) |
|
Closing since this was addressed as #89652 instead! |
paircan have trivial copy and move assignment operators when both its members have trivial copy and move operators, which openspairup to being trivially copyable. This is the first of three possible commits to bring users a more robust implementation ofstd::pair.It's currently unclear how to implement this without using a requires-clause, so the feature is restricted to C++20 for now, but backporting this to C++11 would be ideal. There has also been some interest in moving this out of the unstable ABI mode. Both of these changes are intended to arrive in future pull requests, should their feasibility pan out.