[libc++] Make std::pair trivially copyable if its members are #89652

philnik777 · 2024-04-22T19:02:04Z

This makes std::pair trivially copyable if its members are and we have a way to do so. We need either C++20 with requires clauses or support for __attribute__((enable_if)). Only Clang has support for this attribute, so it's effectively clang or C++20.

Co-authored-by: Christopher Di Bella [email protected]

llvmbot · 2024-04-22T19:02:40Z

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This makes std::pair trivially copyable if its members are and we have a way to do so. We need either C++20 with requires clauses or support for __attribute__((enable_if)). Only Clang has support for this attribute, so its effectively clang or C++20.

Full diff: https://github.com/llvm/llvm-project/pull/89652.diff

2 Files Affected:

(modified) libcxx/include/__utility/pair.h (+34-6)
(added) libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp (+32)

diff --git a/libcxx/include/__utility/pair.h b/libcxx/include/__utility/pair.h
index e05250ba05717f..5a9f9f4a3a07f2 100644
--- a/libcxx/include/__utility/pair.h
+++ b/libcxx/include/__utility/pair.h
@@ -141,6 +141,31 @@ struct _LIBCPP_TEMPLATE_VIS pair
       _NOEXCEPT_(is_nothrow_copy_constructible<first_type>::value&& is_nothrow_copy_constructible<second_type>::value)
       : first(__t1), second(__t2) {}
 
+  // Make pair trivially copyable if we have a way to do it
+  static const bool __enable_defaulted_assignment_operators =
+      !is_reference<first_type>::value && !is_reference<second_type>::value;
+#if _LIBCPP_STD_VER >= 20
+  static const bool __has_defaulted_members = __enable_defaulted_assignment_operators;
+
+  _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(const pair&)
+    requires __enable_defaulted_assignment_operators
+  = default;
+
+  _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(pair&&)
+    requires __enable_defaulted_assignment_operators
+  = default;
+#  elif __has_attribute(__enable_if__)
+  static const bool __has_defaulted_members = __enable_defaulted_assignment_operators;
+
+  _LIBCPP_HIDE_FROM_ABI pair& operator=(const pair&)
+      __attribute__((__enable_if__(__enable_defaulted_assignment_operators, ""))) = default;
+
+  _LIBCPP_HIDE_FROM_ABI pair& operator=(pair&&)
+      __attribute__((__enable_if__(__enable_defaulted_assignment_operators, ""))) = default;
+#else
+  static const bool __has_defaulted_members = false;
+#  endif                    // __has_attribute(__enable_if__)
+
   template <
 #  if _LIBCPP_STD_VER >= 23 // http://wg21.link/P1951
       class _U1 = _T1,
@@ -221,18 +246,21 @@ struct _LIBCPP_TEMPLATE_VIS pair
              typename __make_tuple_indices<sizeof...(_Args2) >::type()) {}
 
   _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,
-                             __nat> const& __p)
+  operator=(__conditional_t<
+            !__has_defaulted_members && is_copy_assignable<first_type>::value && is_copy_assignable<second_type>::value,
+            pair,
+            __nat> const& __p)
       _NOEXCEPT_(is_nothrow_copy_assignable<first_type>::value&& is_nothrow_copy_assignable<second_type>::value) {
     first  = __p.first;
     second = __p.second;
     return *this;
   }
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pair& operator=(
-      __conditional_t< is_move_assignable<first_type>::value && is_move_assignable<second_type>::value, pair, __nat>&&
-          __p)
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 pair&
+  operator=(__conditional_t<
+            !__has_defaulted_members && is_move_assignable<first_type>::value && is_move_assignable<second_type>::value,
+            pair,
+            __nat>&& __p)
       _NOEXCEPT_(is_nothrow_move_assignable<first_type>::value&& is_nothrow_move_assignable<second_type>::value) {
     first  = std::forward<first_type>(__p.first);
     second = std::forward<second_type>(__p.second);
diff --git a/libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp b/libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp
new file mode 100644
index 00000000000000..2f4a77c3112200
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp
@@ -0,0 +1,32 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: c++20 || clang
+
+#include <type_traits>
+#include <utility>
+
+struct trivially_copyable {
+  int arr[4];
+};
+
+static_assert(std::is_trivially_copyable<std::pair<int, int>>::value, "");
+static_assert(std::is_trivially_copyable<std::pair<int, char>>::value, "");
+static_assert(std::is_trivially_copyable<std::pair<char, int>>::value, "");
+static_assert(std::is_trivially_copyable<std::pair<std::pair<char, char>, int>>::value, "");
+static_assert(std::is_trivially_copyable<std::pair<trivially_copyable, int>>::value, "");
+
+static_assert(!std::is_trivially_copyable<std::pair<int&, int>>::value, "");
+static_assert(!std::is_trivially_copyable<std::pair<int, int&>>::value, "");
+static_assert(!std::is_trivially_copyable<std::pair<int&, int&>>::value, "");
+
+static_assert(std::is_trivially_copy_constructible<std::pair<int, int>>::value);
+static_assert(std::is_trivially_move_constructible<std::pair<int, int>>::value);
+static_assert(std::is_trivially_copy_assignable<std::pair<int, int>>::value);
+static_assert(std::is_trivially_move_assignable<std::pair<int, int>>::value);
+static_assert(std::is_trivially_destructible<std::pair<int, int>>::value);

github-actions · 2024-04-22T19:04:53Z

✅ With the latest revision this PR passed the C/C++ code formatter.

philnik777 · 2024-04-22T19:07:13Z

CC @cjdb

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp

mordante · 2024-04-23T18:23:40Z

I've not really looked closely at the changes. Can you provide a rationale in the commit message why we want to do an unconditional ABI break?

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp

philnik777 · 2024-04-23T18:36:31Z

I've not really looked closely at the changes. Can you provide a rationale in the commit message why we want to do an unconditional ABI break?

IMO we should break the ABI if it doesn't actually impact people negatively. The question is mostly whether it will. For this I don't see an obvious failure mode, and it can significantly impact code gen, reducing code size and improving speed (e.g. in std::copy or vector::resize).

mordante · 2024-04-23T18:47:21Z

I've not really looked closely at the changes. Can you provide a rationale in the commit message why we want to do an unconditional ABI break?

IMO we should break the ABI if it doesn't actually impact people negatively. The question is mostly whether it will. For this I don't see an obvious failure mode, and it can significantly impact code gen, reducing code size and improving speed (e.g. in std::copy or vector::resize).

What happens when mixing C++17 and C++20 code, would that work correctly or is there an ABI incompatibility. I would like to have this information in the commit message so the information is in Git; if we wonder in x years why, it's nice to read that in Git.

I'm not per se against the unconditional ABI break, but I really like to know the scope.

libcxx/include/__utility/pair.h

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp

libcxx/include/__utility/pair.h

ldionne

LGTM once CI is green.

libcxx/include/__utility/pair.h

cjdb · 2024-05-31T16:29:01Z

Thanks for working on this. How is it materially different to #84811?

philnik777 · 2024-05-31T16:32:41Z

Thanks for working on this. How is it materially different to #84811?

It's not fundamentally different. The main change is that I use __attribute__((enable_if)) to back-port this to C++03 with clang. If you don't mind I'd add you as a co-author, since I basically stole the idea.

cjdb · 2024-05-31T16:43:00Z

Thanks for working on this. How is it materially different to #84811?

Ah, after a bit of reading, I think I see the difference. IIUC you're going for "make pair<T, U> trivially copyable if T and U are trivially copyable", whereas #84811 does "make pair<T, U> trivially copy constructible if T and U are trivially copy constructible", "make pair<T, U> trivially copy assignable if T and U are trivially copy assignable", etc.

Your approach is simpler, and probably covers the vast majority of cases. I think that's why you can get away with not needing is_object (which is what tipped me off that we're going about this differently).

It's not fundamentally different. The main change is that I use __attribute__((enable_if)) to back-port this to C++03 with clang.

A C++03-friendly approach would be good. I think I was only able to get it back to C++11 before my attention was diverted elsewhere.

If you don't mind I'd add you as a co-author, since I basically stole the idea.

Sure, that is appreciated :)

EricWF

I would like to see some discussion about how the ABI of pair isn't controlled by the ABI macro, but rather the ABI macro, and the dialect, and the compiler. Meaning you can set the macro and get the old ABI.

If we contain std::pair of a trivial type anywhere inside the dylib, the dylib may only be compatible with the compiler it was built with (since GCC doesn't provide the enable if macro).

EricWF · 2024-06-02T21:13:46Z

libcxx/include/__utility/pair.h

+  // When we are requested for pair to be trivially copyable by the ABI macro, we use defaulted members
+  // if it is both legal to do it (i.e. no references) and we have a way to actually implement it, which requires
+  // the __enable_if__ attribute before C++20.
+#if defined(_LIBCPP_ABI_TRIVIALLY_COPYABLE_PAIR) && _LIBCPP_STD_VER >= 20


I worry about shipping this since the ABI is not stable across compilers (GCC before C++20 produces a different ABI).

cjdb · 2024-07-23T17:04:57Z

We're seeing errors to the effect of the following internally:

/tmp/a_file.cc:37:9: error: use of overloaded operator '=' is ambiguous (with operand types 'value_type' (aka 'std::pair<SomethingNotTriviallyCopyable, std::string_view>') and 'void')
   37 |     kv_ = {};
      |     ~~~ ^ ~~
/tmp/libcxx/include/__utility/pair.h:94:41: note: candidate function
   94 |   _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(const pair&)
      |                                         ^
/tmp/libcxx/include/__utility/pair.h:98:41: note: candidate function
   98 |   _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(pair&&)
      |                                         ^
/tmp/libcxx/include/__utility/pair.h:261:3: note: candidate function
  261 |   operator=(__conditional_t<!__has_defaulted_members::value && is_copy_assignable<first_type>::value &&
      |   ^
/tmp/libcxx/include/__utility/pair.h:272:3: note: candidate function
  272 |   operator=(__conditional_t<!__has_defaulted_members::value && is_move_assignable<first_type>::value &&
      |   ^
/tmp/libcxx/include/__utility/pair.h:99:14: note: similar constraint expressions not considered equivalent; constraint expressions cannot be considered equivalent unless they originate from the same concept
   99 |     requires __has_defaulted_members::value
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/tmp/libcxx/include/__utility/pair.h:95:14: note: similar constraint expression here
   95 |     requires __has_defaulted_members::value
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

I think we'll need to roll this back, and I will work with @philnik777 to get it back in ASAP.

nico · 2024-07-23T17:26:36Z

Is this expected?

(Only fires in one place, and it makes sense to fire -- it's a bit similar to std::vector<const T>. So not a big problem; just a bit surprising from this PR's description that it'd have this effect.)

nico · 2024-07-23T17:31:31Z

(Only fires in one place, and it makes sense to fire -- it's a bit similar to std::vector<const T>. So not a big problem; just a bit surprising from this PR's description that it'd have this effect.)

I take that back. After this patch, this fails:

static_assert(
    std::is_trivially_copyable<std::pair<const PhiOp *, const OpIndex>>::value);

And this (no const) succeeds:

static_assert(
    std::is_trivially_copyable<std::pair<const PhiOp *, OpIndex>>::value);

However, before this patch, it's the exact opposite. It succeeds with the const but fails without it.

So there's no way to write code that works with both versions, as far as I can tell (?)

ldionne · 2024-07-23T17:45:40Z

@nico

I believe the new behavior is expected and correct. Indeed, based on https://godbolt.org/z/9TxKevxzn, I see the following behavior (please confirm we're on the same page):

// BEFORE THE PATCH
std::is_trivially_copyable<std::pair<int, OpIndex>>::value == false
std::is_trivially_copyable<std::pair<int, const OpIndex>>::value == false

// AFTER THE PATCH
std::is_trivially_copyable<std::pair<int, OpIndex>>::value == true
std::is_trivially_copyable<std::pair<int, const OpIndex>>::value == false

Going from false to true for the non-const OpIndex case is the purpose of this patch, so that's an intended change. Staying false for the const OpIndex case isn't a change. I don't understand why it's not true after the patch and I'm not certain what's the best behavior within the Standard's constraints, but it doesn't seem like an obvious bug.

ldionne · 2024-07-23T17:49:01Z

@cjdb I am still waiting for a reproducer for the assignment example you showed above, please LMK once you have it.

If we don't manage to figure out whether there's something broken with this patch soon I would err on the side of reverting it for LLVM 19 stability's sake, but let's at least dig a little bit before we do that to ensure we don't revert something that's entirely correct. We know for a fact that this patch is a behavior change, that's why it's behind the ABI flag. That alone shouldn't be enough to mandate a revert. If we do figure out that it's broken, or if we have strong suspicion that it is, then as I said we should revert.

nico · 2024-07-23T18:08:54Z

@nico

I believe the new behavior is expected and correct. Indeed, based on https://godbolt.org/z/9TxKevxzn, I see the following behavior (please confirm we're on the same page):

If I use -std=c++20 insteadof-std=c++23`, I'm seeing the following (https://godbolt.org/z/8eqdz53fT , and #89652 (comment))

// BEFORE THE PATCH
std::is_trivially_copyable<std::pair<int, OpIndex>>::value == false
std::is_trivially_copyable<std::pair<int, const OpIndex>>::value == true

// AFTER THE PATCH
std::is_trivially_copyable<std::pair<int, OpIndex>>::value == true
std::is_trivially_copyable<std::pair<int, const OpIndex>>::value == false

Going from false to true for the non-const OpIndex case is the purpose of this patch, so that's an intended change.

Yep.

Staying false for the const OpIndex case isn't a change. I don't understand why it's not true after the patch and I'm not certain what's the best behavior within the Standard's constraints, but it doesn't seem like an obvious bug.

With -std=c++20, it is a change, from true to false when the second element in the pair is a const OpIndex.

We currently have a custom_container<T> (here) that asserts is_trivially_copyable<T>, and T is a pair<int, const OpIndex>. That currently compiles. After this patch, we have to change that to pair<int, OpIndex> without const – but we can't land that change since it breaks building with libc++ before this change.

cjdb · 2024-07-23T18:24:58Z

That it's changing from standard version to standard version may be due to how the language rules work. We occasionally update the meaning of trivially copyable, and that can impact the result of the trait.

nico · 2024-07-23T18:31:05Z

https://isocpp.org/files/papers/N4860.pdf 20.4.2 Class template pair [pairs.pair]p23 (about the assignment op): "Remarks: This operator is defined as deleted unless is_copy_assignable_v<first_type> is true and
is_copy_assignable_v<second_type> is true."

p28 (on the move assignment op): "Constraints:
(28.1) — is_move_assignable_v<first_type> is true and
(28.2) — is_move_assignable_v<second_type> is true."

The new __has_defaulted_members doesn't honor those. If I make them honor it as below (only did the C++20 branch), my code stays working as-is:

% git diff
diff --git a/libcxx/include/__utility/pair.h b/libcxx/include/__utility/pair.h
index c0002b7abb3c..bc30eb6ca718 100644
--- a/libcxx/include/__utility/pair.h
+++ b/libcxx/include/__utility/pair.h
@@ -92,11 +92,21 @@ struct _LIBCPP_TEMPLATE_VIS pair
   };
 #  if _LIBCPP_STD_VER >= 20
   _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(const pair&)
-    requires __has_defaulted_members::value
+    requires (__has_defaulted_members::value &&
+              is_copy_assignable<first_type>::value &&
+              is_copy_assignable<second_type>::value)
   = default;
 
+  _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(const pair&)
+    requires (__has_defaulted_members::value &&
+              !(is_copy_assignable<first_type>::value &&
+                is_copy_assignable<second_type>::value))
+  = delete;
+
   _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(pair&&)
-    requires __has_defaulted_members::value
+    requires (__has_defaulted_members::value &&
+              is_move_assignable<first_type>::value &&
+              is_move_assignable<second_type>::value)
   = default;
 #  elif __has_attribute(__enable_if__)
   _LIBCPP_HIDE_FROM_ABI pair& operator=(const pair&)

Looks like the PR as-submitted might not be complete, maybe. (I'm not a standards expert though.)

cjdb · 2024-07-23T18:49:56Z

https://isocpp.org/files/papers/N4860.pdf 20.4.2 Class template pair [pairs.pair]p23 (about the assignment op): "Remarks: This operator is defined as deleted unless is_copy_assignable_v<first_type> is true and is_copy_assignable_v<second_type> is true."

p28 (on the move assignment op): "Constraints: (28.1) — is_move_assignable_v<first_type> is true and (28.2) — is_move_assignable_v<second_type> is true."

The new __has_defaulted_members doesn't honor those. If I make them honor it as below (only did the C++20 branch), my code stays working as-is:

% git diff
diff --git a/libcxx/include/__utility/pair.h b/libcxx/include/__utility/pair.h
index c0002b7abb3c..bc30eb6ca718 100644
--- a/libcxx/include/__utility/pair.h
+++ b/libcxx/include/__utility/pair.h
@@ -92,11 +92,21 @@ struct _LIBCPP_TEMPLATE_VIS pair
   };
 #  if _LIBCPP_STD_VER >= 20
   _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(const pair&)
-    requires __has_defaulted_members::value
+    requires (__has_defaulted_members::value &&
+              is_copy_assignable<first_type>::value &&
+              is_copy_assignable<second_type>::value)
   = default;
 
+  _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(const pair&)
+    requires (__has_defaulted_members::value &&
+              !(is_copy_assignable<first_type>::value &&
+                is_copy_assignable<second_type>::value))
+  = delete;
+
   _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(pair&&)
-    requires __has_defaulted_members::value
+    requires (__has_defaulted_members::value &&
+              is_move_assignable<first_type>::value &&
+              is_move_assignable<second_type>::value)
   = default;
 #  elif __has_attribute(__enable_if__)
   _LIBCPP_HIDE_FROM_ABI pair& operator=(const pair&)

Looks like the PR as-submitted might not be complete, maybe. (I'm not a standards expert though.)

Those additions need to be is_trivially_copy_assignable and is_trivially_move_assignable for the defaulted overloads, but otherwise, a forward fix might be on the horizon?

ldionne · 2024-07-23T18:50:41Z

@cjdb Do you have a reproducer for your issue? I am still trying to understand what's the cause of the difference between C++20 and C++23

philnik777 · 2024-07-23T19:03:02Z

I think I see the problem with @cjdb's case. We don't actually drop the non-trivial operator= from overload resolution. We just make it __nat, so we have multiple candidates. I'm not sure why a pair with a const member is not trivially copyable anymore though. An explicitly defaulted special member should just be implicitly deleted.

ldionne · 2024-07-23T19:03:58Z

I'm not sure why a pair with a const member is not trivially copyable anymore though. An explicitly defaulted special member should just be implicitly deleted.

Right? I'm banging my head against that one too.

ldionne · 2024-07-23T19:11:14Z

I'm not sure why a pair with a const member is not trivially copyable anymore though. An explicitly defaulted special member should just be implicitly deleted.

Right? I'm banging my head against that one too.

@philnik777 Here's something strange:

  _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(const pair&)
    requires (__has_defaulted_members::value && is_copy_assignable<first_type>::value && is_copy_assignable<second_type>::value)
  = default;

  _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(const pair&)
    requires (__has_defaulted_members::value && !(is_copy_assignable<first_type>::value && is_copy_assignable<second_type>::value))
  = delete;

  _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(pair&&)
    requires (__has_defaulted_members::value && is_move_assignable<first_type>::value && is_move_assignable<second_type>::value)
  = default;

//   _LIBCPP_HIDE_FROM_ABI constexpr pair& operator=(pair&&)
//     requires (__has_defaulted_members::value && !(is_move_assignable<first_type>::value && is_move_assignable<second_type>::value))
//   = delete;

Using this set of defaulted operators fixes @nico 's example in C++20 and C++23 (see https://godbolt.org/z/8M9xKEMdP). But uncommenting the operator=(pair&&) = delete one breaks it in both standard modes (see https://godbolt.org/z/9oWKxe4fh). I don't understand this.

ldionne · 2024-07-23T19:12:46Z

Would you be OK with reverting the patch for the time being? I think it would be good to have more time to understand what's going on and add missing test coverage.

philnik777 · 2024-07-23T19:17:00Z

Yeah, let's revert. The triviality with const seems weird enough that we should look into it a bit more.

…llvm#89652)" This reverts commit f9dd885. We're not certain yet whether the patch has issues, so we are reverting until we've had time to investigate.

ldionne · 2024-07-23T19:27:10Z

Reverting in #100184.

…#89652)" (#100184) This reverts commit f9dd885. We're not certain yet whether the patch has issues, so we are reverting until we've had time to investigate.

gulfemsavrun · 2024-07-24T08:12:13Z

@gulfemsavrun What even is the failure mode? I've never seen that error.

I know that the patch got reverted, but this is full error message that we get:

[36714/325859](63) CXX host_x64/obj/src/testing/catapult_converter/converter.converter.cc.o
FAILED: host_x64/obj/src/testing/catapult_converter/converter.converter.cc.o 
../../prebuilt/third_party/clang/custom/bin/clang++ -MD -MF host_x64/obj/src/testing/catapult_converter/converter.converter.cc.o.d -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS -D_LIBCPP_DISABLE_VISIBILITY_ANNOTATIONS -D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES -DNDEBUG=1 -DRAPIDJSON_HAS_STDSTRING -DRAPIDJSON_HAS_CXX11_RANGE_FOR -DRAPIDJSON_HAS_CXX11_RVALUE_REFS -DRAPIDJSON_HAS_CXX11_TYPETRAITS -DRAPIDJSON_HAS_CXX11_NOEXCEPT -I../.. -Ihost_x64/gen -I../../sdk -Ihost_x64/gen/sdk -I../../sdk/lib/fit-promise/include -I../../sdk/lib/fit/include -I../../sdk/lib/stdcompat/include -I../../zircon/system/public -I../../zircon/system/ulib/fbl/include -I../../third_party/rapidjson/include -fcolor-diagnostics -fcrash-diagnostics-dir=clang-crashreports -fcrash-diagnostics=all -gen-reproducer=error -ffp-contract=off --sysroot=../../prebuilt/third_party/sysroot/linux --target=x86_64-unknown-linux-gnu -ffile-compilation-dir=. -no-canonical-prefixes -fomit-frame-pointer -fdata-sections -ffunction-sections -Os -Xclang -debug-info-kind=constructor -g3 -grecord-gcc-switches -gdwarf-5 -gz=zstd -Wall -Wextra -Wconversion -Wextra-semi -Wimplicit-fallthrough -Wnewline-eof -Wstrict-prototypes -Wwrite-strings -Wno-sign-conversion -Wno-unused-parameter -Wnonportable-system-include-path -Wno-missing-field-initializers -Wno-extra-qualification -Wno-cast-function-type-strict -Wno-cast-function-type-mismatch -Wno-unknown-warning-option -Wno-missing-template-arg-list-after-template-kw -Wno-deprecated-pragma -fvisibility=hidden -Werror -Wa,--fatal-warnings --sysroot=../../prebuilt/third_party/sysroot/linux --target=x86_64-unknown-linux-gnu -fPIE -Wno-ambiguous-reversed-operator -Wno-option-ignored -fPIC -fvisibility-inlines-hidden -stdlib=libc++ -std=c++17 -fno-exceptions -fno-rtti -ftemplate-backtrace-limit=0 -stdlib=libc++ -c ../../src/testing/catapult_converter/converter.cc -o host_x64/obj/src/testing/catapult_converter/converter.converter.cc.o
In file included from ../../src/testing/catapult_converter/converter.cc:5:
In file included from ../../src/testing/catapult_converter/converter.h:8:
In file included from ../../third_party/rapidjson/include/rapidjson/document.h:20:
In file included from ../../third_party/rapidjson/include/rapidjson/reader.h:20:
In file included from ../../third_party/rapidjson/include/rapidjson/allocators.h:18:
In file included from ../../third_party/rapidjson/include/rapidjson/rapidjson.h:162:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/string:597:
In file included from ../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/__functional/hash.h:20:
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/__utility/pair.h:102:31: error: the parameter for this explicitly-defaulted copy assignment operator is const, but a member or base requires it to be non-const
  102 |   _LIBCPP_HIDE_FROM_ABI pair& operator=(const pair&)
      |                               ^
../../prebuilt/third_party/clang/custom/bin/../include/c++/v1/map:766:14: note: in instantiation of template class 'std::pair<const std::string, rapidjson::GenericValue<rapidjson::UTF8<>>>' requested here
  766 |   value_type __cc_;
      |              ^
../../src/testing/catapult_converter/converter.cc:345:12: note: in instantiation of template class 'std::__value_type<std::string, rapidjson::GenericValue<rapidjson::UTF8<>>>' requested here
  345 |     if (it != test_suite_to_guid.end()) {
      |            ^

https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8741574608160376113/overview

ldionne · 2024-07-24T15:35:04Z

Thanks. We will re-land the patch, so this is useful information.

This makes `std::pair` trivially copyable if its members are and we have a way to do so. We need either C++20 with requires clauses or support for `__attribute__((enable_if))`. Only Clang has support for this attribute, so it's effectively clang or C++20. Co-authored-by: Christopher Di Bella <[email protected]>

…#89652)" (#100184) Summary: This reverts commit f9dd885. We're not certain yet whether the patch has issues, so we are reverting until we've had time to investigate. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250646

llvm/llvm-project#89652 made std::pair trivially copyable if its members are, and it caused the following issue: "error: the parameter for this explicitly-defaulted copy assignment operator is const, but a member or base requires it to be non-const". This patch adds const to copy assignment operator to fix the issue. Bug: 354627057 Change-Id: I10ee04c40da8820e33b80f51492e136a27e97972 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1086537 Fuchsia-Auto-Submit: Gulfem Savrun Yeniceri <[email protected]> Commit-Queue: Auto-Submit <[email protected]> Reviewed-by: John Bauman <[email protected]>

llvm/llvm-project#89652 made std::pair trivially copyable if its members are, and it caused the following issue: "error: the parameter for this explicitly-defaulted copy assignment operator is const, but a member or base requires it to be non-const". This patch adds const to copy assignment operator to fix the issue. Bug: 354627057 Change-Id: Ic3b4db830502c9a1ef5260e7b15dd24cb9ced783 Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1086536 Fuchsia-Auto-Submit: Gulfem Savrun Yeniceri <[email protected]> Reviewed-by: Matt Thiffault <[email protected]> Commit-Queue: Auto-Submit <[email protected]>

llvm/llvm-project#89652 made std::pair trivially copyable if its members are, and it caused the following issue: "error: the parameter for this explicitly-defaulted copy assignment operator is const, but a member or base requires it to be non-const". This patch adds const to copy assignment operator to fix the issue. Original-Bug: 354627057 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1086537 Original-Revision: 3d310ae50b6ea4982450cdb6f817fd2acfb02ead GitOrigin-RevId: 25df2142c8e3b35418b44dc7ffa7afe524d0dd41 Change-Id: Ie60e29c2d9120a8dd6e1c1989e8451b6c7c45962

llvm/llvm-project#89652 made std::pair trivially copyable if its members are, and it caused the following issue: "error: the parameter for this explicitly-defaulted copy assignment operator is const, but a member or base requires it to be non-const". This patch adds const to copy assignment operator to fix the issue. Original-Bug: 354627057 Original-Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1086536 Original-Revision: a2fef69a6c032a50ac2c62ead4e7212a488ad238 GitOrigin-RevId: 18c0605dc1feadcd7020552002db782a3b26fd18 Change-Id: I74457afae466f7e65aecf1dffe938dc02ab8062b

…llvm#89652)" (llvm#100184) This reverts commit f9dd885. We're not certain yet whether the patch has issues, so we are reverting until we've had time to investigate.

danlark1 · 2024-09-05T15:37:18Z

Is there any plan to reland this? We (Google) can offer help

nico · 2024-09-06T13:09:58Z

I think the first step is to figure out why the behavior differs in C++20 and C++23: #89652 (comment)

philnik777 requested a review from a team as a code owner April 22, 2024 19:02

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 22, 2024

philnik777 force-pushed the trivially_copyable_pair branch from 90e2029 to b06140e Compare April 22, 2024 19:09

frederick-vs-ja reviewed Apr 23, 2024

View reviewed changes

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp Outdated Show resolved Hide resolved

philnik777 force-pushed the trivially_copyable_pair branch 2 times, most recently from 3c390aa to 6347173 Compare April 23, 2024 08:17

EricWF reviewed Apr 23, 2024

View reviewed changes

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp Outdated Show resolved Hide resolved

ldionne requested changes Apr 25, 2024

View reviewed changes

libcxx/include/__utility/pair.h Outdated Show resolved Hide resolved

libcxx/test/libcxx/utilities/utility/pairs/pairs.pair/trivially_copyable.compile.pass.cpp Outdated Show resolved Hide resolved

libcxx/include/__utility/pair.h Show resolved Hide resolved

ldionne self-assigned this Apr 26, 2024

philnik777 force-pushed the trivially_copyable_pair branch from 6347173 to 9a4773d Compare April 30, 2024 12:33

philnik777 changed the title ~~[libc++][ABI BREAK] Make std::pair trivially copyable if its members are~~ [libc++] Make std::pair trivially copyable if its members are May 2, 2024

philnik777 force-pushed the trivially_copyable_pair branch 2 times, most recently from 4550098 to 2384d39 Compare May 2, 2024 11:02

ldionne reviewed May 9, 2024

View reviewed changes

libcxx/include/__utility/pair.h Outdated Show resolved Hide resolved

libcxx/include/__utility/pair.h Outdated Show resolved Hide resolved

libcxx/include/__utility/pair.h Outdated Show resolved Hide resolved

libcxx/include/__utility/pair.h Outdated Show resolved Hide resolved

philnik777 force-pushed the trivially_copyable_pair branch from 2384d39 to 0581ba7 Compare May 11, 2024 20:30

ldionne previously approved these changes May 30, 2024

View reviewed changes

libcxx/include/__utility/pair.h Outdated Show resolved Hide resolved

philnik777 force-pushed the trivially_copyable_pair branch 4 times, most recently from a38e80c to e4ab57b Compare June 2, 2024 19:14

EricWF previously requested changes Jun 2, 2024

View reviewed changes

ldionne mentioned this pull request Jul 31, 2024

[libcxx] makes pair conditionally trivially copyable for C++20 #84811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Make std::pair trivially copyable if its members are #89652

[libc++] Make std::pair trivially copyable if its members are #89652

philnik777 commented Apr 22, 2024 •

edited

Loading

llvmbot commented Apr 22, 2024

github-actions bot commented Apr 22, 2024 •

edited

Loading

philnik777 commented Apr 22, 2024

mordante commented Apr 23, 2024

philnik777 commented Apr 23, 2024

mordante commented Apr 23, 2024

ldionne left a comment

cjdb commented May 31, 2024

philnik777 commented May 31, 2024

cjdb commented May 31, 2024

EricWF left a comment

EricWF Jun 2, 2024

cjdb commented Jul 23, 2024

nico commented Jul 23, 2024

nico commented Jul 23, 2024 •

edited by ldionne

Loading

ldionne commented Jul 23, 2024 •

edited

Loading

ldionne commented Jul 23, 2024

nico commented Jul 23, 2024 •

edited

Loading

cjdb commented Jul 23, 2024

nico commented Jul 23, 2024 •

edited

Loading

cjdb commented Jul 23, 2024 •

edited

Loading

ldionne commented Jul 23, 2024

philnik777 commented Jul 23, 2024

ldionne commented Jul 23, 2024 •

edited

Loading

ldionne commented Jul 23, 2024

ldionne commented Jul 23, 2024

philnik777 commented Jul 23, 2024

ldionne commented Jul 23, 2024

gulfemsavrun commented Jul 24, 2024

ldionne commented Jul 24, 2024

danlark1 commented Sep 5, 2024

nico commented Sep 6, 2024 •

edited

Loading

[libc++] Make std::pair trivially copyable if its members are #89652

[libc++] Make std::pair trivially copyable if its members are #89652

Conversation

philnik777 commented Apr 22, 2024 • edited Loading

llvmbot commented Apr 22, 2024

github-actions bot commented Apr 22, 2024 • edited Loading

philnik777 commented Apr 22, 2024

mordante commented Apr 23, 2024

philnik777 commented Apr 23, 2024

mordante commented Apr 23, 2024

ldionne left a comment

Choose a reason for hiding this comment

cjdb commented May 31, 2024

philnik777 commented May 31, 2024

cjdb commented May 31, 2024

EricWF left a comment

Choose a reason for hiding this comment

EricWF Jun 2, 2024

Choose a reason for hiding this comment

cjdb commented Jul 23, 2024

nico commented Jul 23, 2024

nico commented Jul 23, 2024 • edited by ldionne Loading

ldionne commented Jul 23, 2024 • edited Loading

ldionne commented Jul 23, 2024

nico commented Jul 23, 2024 • edited Loading

cjdb commented Jul 23, 2024

nico commented Jul 23, 2024 • edited Loading

cjdb commented Jul 23, 2024 • edited Loading

ldionne commented Jul 23, 2024

philnik777 commented Jul 23, 2024

ldionne commented Jul 23, 2024 • edited Loading

ldionne commented Jul 23, 2024

ldionne commented Jul 23, 2024

philnik777 commented Jul 23, 2024

ldionne commented Jul 23, 2024

gulfemsavrun commented Jul 24, 2024

ldionne commented Jul 24, 2024

danlark1 commented Sep 5, 2024

nico commented Sep 6, 2024 • edited Loading

philnik777 commented Apr 22, 2024 •

edited

Loading

github-actions bot commented Apr 22, 2024 •

edited

Loading

nico commented Jul 23, 2024 •

edited by ldionne

Loading

ldionne commented Jul 23, 2024 •

edited

Loading

nico commented Jul 23, 2024 •

edited

Loading

nico commented Jul 23, 2024 •

edited

Loading

cjdb commented Jul 23, 2024 •

edited

Loading

ldionne commented Jul 23, 2024 •

edited

Loading

nico commented Sep 6, 2024 •

edited

Loading