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

[Louis Dionne via libcxx-commits]

ldionne wrote:

I am a bit late to the party, but let me document my understanding of the situation and explain why I had approved the patch.

**First**, there's the question of whether this should be hidden behind an ABI macro or not. Based on searching for `is_trivially_copyable` in open source libraries, we found a few instances of libraries like Folly who change the layout of structs based on it. This is only the tip of the iceberg and since we can easily find a few things that get broken by this, my opinion is that it's not safe to make the change without an ABI macro. That is why I requested to hide this feature behind an ABI macro in earlier reviews, which was done. We can re-discuss this if needed, but based on the research we did so far I would say I am strongly leaning towards the current approach (i.e. hide behind ABI macro).

**Second**, there's the question of what our ABI macros mean. Namely, there are two possible interpretations:
1. Each ABI macro defines a new separate ABI which must be stable in its own right. This means that when you define that ABI macro, you get a library configured with that "ABI feature" and that "configured library instance" must provide the same ABI stability as the default ABI: it must be stable across all supported compilers, all supported language modes and minor flag variations like `-fno-exceptions`, `-fno-rtti`, etc.
2. We have the default ABI which is stable, and then we have all the other ABI flags which basically void the notion of ABI stability or compatibility across compilers, dialects, etc. In other words, whenever you opt into an ABI macro that isn't enabled by default (which is all of them, I think), you're saying that you don't care about ABI stability / compatibility at all.

I always thought that the library was operating under the model described by (1). However, after speaking to different people about it, it seems that some folks had always understood it as being (2) -- so this wasn't universally clear like I thought it was. Furthermore, it seems that we already have some instances of ABI macros in libc++ that break the (1) model.

I still think that (1) is the model we want to go for. Or perhaps we want to strike some kind of hybrid between (1) and (2) where we document which ABI flags produce a stable ABI, but where some ABI flags are still allowed to be unstable across compilers (otherwise this limits what we can do mainly due to GCC/Clang differences). However, in both cases it seemed to me that this would require an overhaul of the ABI selection mechanism and its documentation, which I have started looking into locally and for which 23e1ed65c2c3eb1f80f7eeb4897ec843492c5451 was the motivation.

Since I didn't feel like this made the status quo significantly worse (given we already have at least one ABI macro that is similarly non-stable across compilers), I approved the patch and took it upon myself to clarify the meaning of our ABI macros.

I hope this makes sense, and I think it's certainly reasonable to finish this discussion before we move forward with this patch.

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