[libc++] Make std::allocator always trivially default constructible

[philnik777]

This is technically ABI breaking, since is_trivial and is_trivially_default_constructible now return different results. However, I don't think that's a significant issue, since allocator is almost always used in classes which own memory, making them non-trivial anyways.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This is technically ABI breaking, since is_trivial and is_trivially_default_constructible now return different results. However, I don't think that's a significant issue, since allocator is almost always used in classes which own memory, making them non-trivial anyways.

---

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

3 Files Affected:

• (modified) libcxx/include/__memory/allocator.h (+1-29)
• (modified) libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit.pass.cpp (-3)
• (modified) libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit_initializer_list.pass.cpp (-3)

diff --git a/libcxx/include/__memory/allocator.h b/libcxx/include/__memory/allocator.h
index 52f4122a9bf5f..827d37aae1854 100644
--- a/libcxx/include/__memory/allocator.h
+++ b/libcxx/include/__memory/allocator.h
@@ -14,14 +14,11 @@
 #include <__cstddef/ptrdiff_t.h>
 #include <__cstddef/size_t.h>
 #include <__memory/addressof.h>
-#include <__memory/allocate_at_least.h>
 #include <__memory/allocator_traits.h>
 #include <__new/allocate.h>
 #include <__new/exceptions.h>
 #include <__type_traits/is_const.h>
 #include <__type_traits/is_constant_evaluated.h>
-#include <__type_traits/is_same.h>
-#include <__type_traits/is_void.h>
 #include <__type_traits/is_volatile.h>
 #include <__utility/forward.h>
 
@@ -51,33 +48,8 @@ class allocator<void> {
 };
 #endif // _LIBCPP_STD_VER <= 17
 
-// This class provides a non-trivial default constructor to the class that derives from it
-// if the condition is satisfied.
-//
-// The second template parameter exists to allow giving a unique type to __non_trivial_if,
-// which makes it possible to avoid breaking the ABI when making this a base class of an
-// existing class. Without that, imagine we have classes D1 and D2, both of which used to
-// have no base classes, but which now derive from __non_trivial_if. The layout of a class
-// that inherits from both D1 and D2 will change because the two __non_trivial_if base
-// classes are not allowed to share the same address.
-//
-// By making those __non_trivial_if base classes unique, we work around this problem and
-// it is safe to start deriving from __non_trivial_if in existing classes.
-template <bool _Cond, class _Unique>
-struct __non_trivial_if {};
-
-template <class _Unique>
-struct __non_trivial_if<true, _Unique> {
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __non_trivial_if() _NOEXCEPT {}
-};
-
-// allocator
-//
-// Note: For ABI compatibility between C++20 and previous standards, we make
-//       allocator<void> trivial in C++20.
-
 template <class _Tp>
-class allocator : private __non_trivial_if<!is_void<_Tp>::value, allocator<_Tp> > {
+class allocator {
   static_assert(!is_const<_Tp>::value, "std::allocator does not support const types");
   static_assert(!is_volatile<_Tp>::value, "std::allocator does not support volatile types");
 
diff --git a/libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit.pass.cpp b/libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit.pass.cpp
index 5dd1d6f0b3380..b08fce2b701e2 100644
--- a/libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit.pass.cpp
@@ -12,9 +12,6 @@
 // template <class T, class... Args>
 //   constexpr optional<T> make_optional(Args&&... args);
 
-// GCC crashes on this file, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120577
-// XFAIL: gcc-15
-
 #include <cassert>
 #include <memory>
 #include <optional>
diff --git a/libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit_initializer_list.pass.cpp b/libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit_initializer_list.pass.cpp
index 5ddb229ad9268..80371d6333712 100644
--- a/libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit_initializer_list.pass.cpp
+++ b/libcxx/test/std/utilities/optional/optional.specalg/make_optional_explicit_initializer_list.pass.cpp
@@ -12,9 +12,6 @@
 // template <class T, class U, class... Args>
 //   constexpr optional<T> make_optional(initializer_list<U> il, Args&&... args);
 
-// GCC crashes on this file, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=120577
-// XFAIL: gcc-15
-
 #include <cassert>
 #include <memory>
 #include <optional>

[ldionne]

I think this is reasonable. If a type was previously non-trivial, code would call its constructor explicitly in order to be correct. So if we switch to not requiring these calls to be made, that should be correct.

This also doesn't impact whether a type is considered trivial for the purpose of calls, so there's no ABI impact on that side of things.

Basically, I think this can only break if someone is relying on std::is_trivial for changing the layout of their own type, and it should be a rare occurrence for this to happen for std::allocator. I expect it might happen for a container type, but as you said, the container type itself is probably non-trivial already due to other reasons, so this patch doesn't change anything.

[ldionne]

I'd also like to keep an opt-out for the previous behavior for two releases, to give time for people to figure this out and give feedback (i.e. complain) if this ends up having wider fallout than anticipated.

[github-actions]

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