[libcxx] Make removed allocator members constexpr in C++20

[ilya-biryukov]

With -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS, restored allocator members were non-constexpr.
This led to vector and string being non-constexpr too.

This change makes sure the library types stay constexpr even in this backwards-compatibility mode.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Ilya Biryukov (ilya-biryukov)

Changes

With -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS, restored allocator members were non-constexpr.
This led to vector and string being non-constexpr too.

This change makes sure the library types stay constexpr even in this backwards-compatibility mode.

---

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

2 Files Affected:

• (modified) libcxx/include/__memory/allocator.h (+8-7)
• (added) libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/constexpr.cxx2a.verify.cpp (+42)

diff --git a/libcxx/include/__memory/allocator.h b/libcxx/include/__memory/allocator.h
index 47e1ef926a4afe4..3b51984012b7d70 100644
--- a/libcxx/include/__memory/allocator.h
+++ b/libcxx/include/__memory/allocator.h
@@ -152,23 +152,24 @@ class _LIBCPP_TEMPLATE_VIS allocator
         return _VSTD::addressof(__x);
     }
 
-    _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY _LIBCPP_DEPRECATED_IN_CXX17
-    _Tp* allocate(size_t __n, const void*) {
+    _LIBCPP_NODISCARD_AFTER_CXX17 _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_INLINE_VISIBILITY _LIBCPP_DEPRECATED_IN_CXX17
+        _Tp*
+        allocate(size_t __n, const void*) {
         return allocate(__n);
     }
 
-    _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_INLINE_VISIBILITY size_type max_size() const _NOEXCEPT {
+    _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_INLINE_VISIBILITY size_type
+    max_size() const _NOEXCEPT {
         return size_type(~0) / sizeof(_Tp);
     }
 
     template <class _Up, class... _Args>
-    _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_INLINE_VISIBILITY
-    void construct(_Up* __p, _Args&&... __args) {
+    _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_INLINE_VISIBILITY void
+    construct(_Up* __p, _Args&&... __args) {
         ::new ((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
     }
 
-    _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_INLINE_VISIBILITY
-    void destroy(pointer __p) {
+    _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_INLINE_VISIBILITY void destroy(pointer __p) {
         __p->~_Tp();
     }
 #endif
diff --git a/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/constexpr.cxx2a.verify.cpp b/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/constexpr.cxx2a.verify.cpp
new file mode 100644
index 000000000000000..52b76e81ab69b9f
--- /dev/null
+++ b/libcxx/test/libcxx/depr/depr.default.allocator/allocator.members/constexpr.cxx2a.verify.cpp
@@ -0,0 +1,42 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+//  In C++20, parts of std::allocator<T> have been removed.
+//  However, for backwards compatibility, if _LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
+//  is defined before including <memory>, then removed members will be restored.
+
+//  This leads to `vector` and `string` using those members instead of `allocator_traits`.
+//  So the restored members must also be made `constexpr` to make sure instantiated members
+//  of those types stay `constexpr`.
+
+//  Check that the necessary members, vector and string can be used as constants in this mode.
+
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_ENABLE_CXX20_REMOVED_ALLOCATOR_MEMBERS
+// ADDITIONAL_COMPILE_FLAGS: -D_LIBCPP_DISABLE_DEPRECATION_WARNINGS
+// expected-no-diagnostics
+
+#include <memory>
+#include <vector>
+#include <string>
+
+static_assert(std::allocator<int>().max_size() != 0);
+
+constexpr int check_lifetime_return_123() {
+  std::allocator<int> a;
+  int* p = a.allocate(10);
+  a.construct(p, 1);
+  a.destroy(p);
+  a.deallocate(p, 10);
+  return 123;
+}
+static_assert(check_lifetime_return_123() == 123);
+
+static_assert(std::vector<int>({1, 2, 3}).size() == 3);
+static_assert(std::string("abc").size() == 3);

[ldionne]

I don't think this is desirable -- we are just digging deeper and deeper into non-conformance land.

Instead, we should be working towards removing these backward compatibility shims -- this is overdue. @ilya-biryukov in what context are you still enabling these macros?

[ilya-biryukov]

We switched google to C++20, but enabled this flag to postpone the cleanup that gets rid of these members.

I do not see how this digs deeper into non-conformance land since this mode is already non-conformant and the corresponding members in allocator_traits are constexpr. It makes most sense to have the members available in parallel on instances of allocator to have the same signatures (including constexpr).
This change makes this non-standard mode more conformant as vector and string will become constexpr, just like the standard requires.

That being said, rolling this out also causes a lot of breakages because of more constexpr inside our codebase. In particular, Clang starts instantiating members of vector earlier causing "incomplete type" errors all over the place. So doing the cleanup and not landing the patch is the only way forward for us as well.

[ldionne]

That being said, rolling this out also causes a lot of breakages because of more constexpr inside our codebase. In particular, Clang starts instantiating members of vector earlier causing "incomplete type" errors all over the place. So doing the cleanup and not landing the patch is the only way forward for us as well.

This is the kind of stuff I mean when I say "deeper into non-conformance land". Often a seemingly simple change will have unexpected effects like this. And the worst part is that sometimes those effects can be discovered only later once it's really difficult to undo a decision. This is why you will often see me pushing back when people request anything that takes us away from strict conformance (except in specific areas like extension-nodiscard) -- because that usually leads to just more problems down the road. At the end of the day, each Standard version is designed to be a coherent whole and trying to mix things cross-standards or have divergences usually leads to unforeseen issues.

[ilya-biryukov]

I totally see your point and I respect the decision to avoid introducing more non-standard behavior. However, I believe it should apply when someone introduces new flags (or, rather, does not introduce them if that's the policy). For existing flags that produce non-compliant modes, like this one, I do not see a problem with changing the behavior, especially when it moves the code closer to being standard-compliant.

We managed to postpone dealing with those breakages I mentioned above and switched to C++20 earlier, enabling experiments with coroutines while we keep grinding through the cleanups. Those issues I mentioned are not caused by the flag, they are a part of a cleanup when moving from C++17 to C++20. So this flag is still of overall benefit for us, allowing to move more gradually than a full switch from one major language version to another.