[libcxx-commits] [libcxx] [libc++] Make std::pair trivially copyable if its members are (PR #89652)

[Nikolas Klauser via libcxx-commits]

https://github.com/philnik777 commented:

> > @EricWF This is a pre-existing problem. `_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI` and `_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI` do the same thing, so I don't think this should be blocking for this PR. We basically don't support the unstable ABI and GCC currently.
> 
> Another couple of pointst:
> 
>     * It's not the unstable ABI, it's ABI v2, which has been made unstable under GCC.
> 
> 
> I think the discussion should block this PR. If there's no discussion to be had because other people don't care, then that's fine. But continuing to break things, because previous changes broke things before, is not convincing.
> 
> If there was a discussion at the time of the original commits about this, then we can move forward. But if there was no discussion, then it seems those breakages were done out of ignorance and they shouldn't be used as a shield here.
> 
> Now I expect we have few GCC users, and of those, most don't care about ABI stability. But again, ignorance here isn't a defence.
> 
> @philnik777 Was this issues discussed on the previous changes?

I don't think they were discussed in previous PRs. However, I discussed this extensively with Louis privately, and the outcome was that we need to overhaul our ABI macros.

> > @EricWF This is a pre-existing problem. `_LIBCPP_ABI_ENABLE_UNIQUE_PTR_TRIVIAL_ABI` and `_LIBCPP_ABI_ENABLE_SHARED_PTR_TRIVIAL_ABI` do the same thing, so I don't think this should be blocking for this PR. We basically don't support the unstable ABI and GCC currently.
> 
> One more addition: This isn't like those other breakages @philnik777 .
> 
> Those other breakages were only across compiler. With GCC, **_there is a different ABI between C++17 and C++20._**.

This is also pre-existing with `_LIBCPP_ABI_NO_ITERATOR_BASES`.

> Please don't be so fast to dismiss concerns.

I'm not at all dismissing the concerns. In fact, as stated above, Louis and I have talked about this extensively (I realize that that's not something visible to others). I just don't think that these pre-existing problems should block this patch.



> OK, so I think these "ABI" breaks are more benign that I initially thought, since itanium uses doesn't consider the assignment operators for the purposes of the calling convention.
> 
> So I'm happy to unblock on this, though I'm curious now why we're treating this as an ABI break at all? The bytes don't change meaning, and the calling conventions stay the same.

We treat this as an ABI break because there are many types out there that change their representation based on whether a type is considered trivially copyable or not. I'd like to patch this without the ABI flag as well, but Louis thought it was to risky given the ubiquity of `pair`.


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