[PATCH] D143891: [Clang] Adjust triviality computation in QualType::isTrivialType to C++20 cases.

Tue Feb 14 06:49:05 PST 2023

erichkeane added a comment.

In D143891#4125954 <https://reviews.llvm.org/D143891#4125954>, @erichkeane wrote:

> In D143891#4122731 <https://reviews.llvm.org/D143891#4122731>, @aaron.ballman wrote:
>
>> In D143891#4122668 <https://reviews.llvm.org/D143891#4122668>, @royjacobson wrote:
>>
>>> In D143891#4122660 <https://reviews.llvm.org/D143891#4122660>, @aaron.ballman wrote:
>>>
>>>> This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)
>>>
>>> Technically it is, but it only affects code that relies on constrained default constructors, which we're only going to support in Clang 16. So if we backport this to 16 it's not very problematic.
>>
>> Hmmm, if it's true that this only changes the behavior of a type with a constrained default constructor, then I think it's fine despite being an ABI break (we never claimed full support for concepts, so anyone relying on a particular behavior was mistaken to do that). I can't quite convince myself this doesn't impact other cases though -- the logic for computing triviality is nontrivial itself (pun intended), so I'm really only concerned that `__is_trivial` and friends return a different value for a non-constrained type. However, I also can't come up with a test case whose behavior changes either.
>
> Avoiding hte rest of the discussion, because I cannot even:
> ABI Breaks like this are typically OK, BUT we need to update the ClangABIVersion stuff, which includes only implementing this in the newest version.

Aaron points out offline that this is Concepts specific, and can only be run into with a concept.  I didn't realize on first read the 'ineligible' was 'disabled with a constraint'.  If so, we don't really make any guarantees about ABI compat with Concepts yet, so the ClangABIVersion stuff is likely unnecessary.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143891/new/

https://reviews.llvm.org/D143891